[Freeciv-Dev] Re: new_natural_city_names updated again (PR#1127)

[Jason Short <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

per@xxxxxxxxxxx wrote:
> On Tue, 9 Apr 2002, Jason Short wrote:
>
> > This is another update of the new natural city names patch to current
> > CVS.  Nothing has really changed; see all the old discussion for an
> > explanation.
>
> +struct city_name {
> +       char* name;
> +       int river;
> +       int terrain[T_COUNT];
> +};
>
> Can you make it char name[MAX_LEN_NAME] instead? Fewer allocations
> better.
Are you sure?  This will take more memory on average (even considering 
the overhead for a malloc), and will make the termination code (even) 
more ugly (the city_name array is currently terminated by a NULL name).
> -  char **default_city_names;
> -  char **default_rcity_names;          /* river city names */
> -  char **default_crcity_names;         /* coastal-river city names */
> -  char **default_ccity_names;          /* coastal city names */
> -  char **default_tcity_names[T_COUNT]; /* terrain-specific city names */
> +  struct city_name *city_names;                /* The default city names. */
>
> Thank you. I hated those.
Indeed :-).

> +      } while (name && *name);
>
> I hate to say it, because I like this a lot myself, but this should be
>
>   while (name != NULL && strlen(name) > 0)
See other responses.  And although strlen(name) > 0 is much more 
readible, it really seems unnecessary to check the *entire length* of 
the string when all we're interested in is the first character.
> +  GEN_BOOL("naturalcitynames", game.natural_city_names,
> +           SSET_RULES_FLEXIBLE, SSET_TO_CLIENT,
> +           N_("Whether to use natural city names"),
> +           N_("If enabled, the default city names will be determined based "
> +              "on the surrounding terrain."),
> +           NULL, GAME_DEFAULT_NATURALCITYNAMES)
> +
>
> IMHO, you can just make this the default. Why wouldn't anyone want this?
> (And even if they didn't want it, would they bother enough to turn it
> off? I doubt it.) I don't like unnecessary "options creep".
Well, it is the "default".

It would be fine IMO to remove the server option.  But since it was so 
easy I added it in.  The reason someone might not want this is that it 
does tend to change the capital city's name (although within reason, 
unlike the original natural_city_names patch, and it can be manually 
changed back).  Anyone else with a preference?
-----

As for deallocating the city_names array: yes I will add it.   Good catch.

jason