[Freeciv-Dev] Re: [PATCH] Names in rulesets. (PR#559)

[David Pfitzner <dwp@xxxxxxxxxxxxxx>]

gs234@xxxxxxxxx (Gaute B. Strokkenes) wrote:

> Content-Disposition: attachment; filename=ruleset_names-2.diff

> -  if ((!(*name)) || (0 == strcmp(name, "none")) || (0 == strcmp(name, "no")))
> -    {
> -      return (T_LAST);
> -    }
> -  else if (0 == strcmp(name, "yes"))
> -    {
> -      return (tthis);
> -    }
> +  if (!(*name) || !strcmp(name, "none") || !strcmp(name, "no")) {
> +    return T_LAST;
> +  } else if (!strcmp(name, "yes")) {
> +    return tthis;
> +  }

Personally, I much prefer comparing strcmp() to zero rather than using
!strcmp, purely for "mnemonic" value: If strcmp EQUALS 0 then the
strings are EQUAL (the difference is 0), whereas if NOT strcmp, then
they're NOT equal - oops, not!

(I wouldn't necessarily comment about this, except that in some parts
of patch (as above) you're only making this change + formatting, and
we don't generally like formatting-only changes.)

>  /**************************************************************************
> +  Check that a name is not too long, and possibly warn about
> +  truncation, but do not perform the actual truncation.  Return 1 or 0
> +  depending on whether the name should be truncated or not.
> +**************************************************************************/
> +
> +static int check_name(const char *name)
> +{
> +  if (strlen(name) >= MAX_LEN_NAME) {
> +    freelog(LOG_ERROR, "Name too long; truncating: %s", name);
> +    return 1;
> +  }
> +  return 0;
> +}

One thing I don't like about this is that it uses the constant
MAX_LEN_NAME unrelated to name.  That is, it only works for
MAX_LEN_NAME strings, but would be rather easy to accidently use 
for different-size strings as well.  IMO better would be, eg:

   int check_name(const char *name, size_t len)
and 

#define sz_check_name(name) check_name((name), sizeof(name))

-- David