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

[gs234@xxxxxxxxx (Gaute B. Strokkenes)]

David Pfitzner <dwp@xxxxxxxxxxxxxx> writes:

> 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!

There are so many functions that return 0 on success and true on
failure that this has never bothered me.  I guess that the thing that
bothered me about the above is the fact that when I see 'strcmp'
without a leading '!', I expect it to be 'if not equal' and then I
don't notice the trailing '==0' before it's too late.  [Side note:
Sometimes I think that everyone should write their comparisons the
other way round, i.e. '1 == foo' rather than 'foo == 1' to avoid
accidental assignments.  Unfortunately, it just looks too damn odd for
most people, myself included, to get to grips with.]  All the
redundant parentheses really got to me too.

> (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.)

I think I rewrote some bits of this function initially, but then
changed my mind.  I didn't revert the changes because they ended up in
the same chunk as some other stuff, so there was no easy way to get
rid of them short of hand-editing the file to match the original.  It
didn't seem worth it.  (If anyone knows of an easier way, I'd love to
hear from them.)  I don't actively reformat stuff (no matter how
tempting,) but I don't go out of my way to avoid them creeping in,
either.

I think you guys should just have a flag day, run all of Freeciv
through indent, and then enforce style standards with an iron fist.
Seriously.  The source is currently a patch-o-rama of different coding
styles that do not mix very well.  K&R 2 is not my personal favourite
either, but at least it would be consistent.

> 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.

Not really.  Firstly, it's static, so it's only available in
ruleset.c.  Secondly, the name 'check_name' is intended to be mnemonic
(name -> NAME.)  Thirdly, the error message doesn't make sense if the
limit is something other than MAX_LEN_NAME.

I suppose a generic check-and-warn-about-pending-truncation function
might be useful, though.  Something like:
    int check_strlen(const char *str, size_t maxlen, const char *errmsg);
and then
    static const char name_errmsg[] = N_("Name too long, truncating: %s");
    #define check_name(name) check_strlen((str), MAX_LEN_NAME, _(name_errmsg))
in ruleset.c.

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

I'm not sure how useful that would be.  If you've already copied it
into a fixed-size buffer, it had better be big enough.

-- 
Big Gaute (not to be confused with LG)
I'm using my X-RAY VISION to obtain a rare glimpse of the
 INNER WORKINGS of this POTATO!!