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

Kylene Jo Hall
2006-07-14T20:01:38Z

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?