Re: [RFC][PATCH 3/6] SLIM main patch

[Kylene Jo Hall]

On Fri, 2006-07-14 at 11:27 -0700, Dave Hansen wrote:

> > +static enum slm_iac_level set_iac(char *token)
> > +{
> > +	int iac;
> > +
> > +	if (memcmp(token, EXEMPT_STR, strlen(EXEMPT_STR)) == 0)
> > +		return SLM_IAC_EXEMPT;
> > +	else {
> 
> Might as well add brackets here.  Or, just kill the else{} block and
> pull the code back to the lower indenting level.  The else is really
> unnecessary because of the return;
> 
> > +		for (iac = 0; iac < sizeof(slm_iac_str) / sizeof(char *); iac++) {
> > +			if (memcmp(token, slm_iac_str[iac],
> > +				   strlen(slm_iac_str[iac])) == 0)
> > +				return iac;
> 
> Why not use strcmp?
> 
> > +static enum slm_sac_level set_sac(char *token)
> > +{
> > +	int sac;
> > +
> > +	if (memcmp(token, EXEMPT_STR, strlen(EXEMPT_STR)) == 0)
> > +		return SLM_SAC_EXEMPT;
> > +	else {
> > +		for (sac = 0; sac < sizeof(slm_sac_str) / sizeof(char *); sac++) {
> > +			if (memcmp(token, slm_sac_str[sac],
> > +				   strlen(slm_sac_str[sac])) == 0)
> > +				return sac;
> > +		}
> > +	}
> > +	return SLM_SAC_ERROR;
> > +}
> 
> This function looks awfully similar :).  Can you just pass that array in
> as an argument, and get rid of one of the functions?
> 
On closer look combining these would require collapsing them into one
enum or returning int and doing a bunch of casting.  Kind of seems to
void the point of using an enum.  Thus I propose leaving them as is,
okay?


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/