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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Apr 10, 2002 at 12:16:49AM -0700, Jason Short wrote:
> (Note: I tried to send this to the list before, but I sent it to 
> bugs@freeciv and it never showed up on freeciv-dev.  I suspect the 
> reason was that the attachment was way to big (I shouldn't have included 
> one so big), which could explain the rumours of disappearing posts sent 
> to bugs@freeciv.)
> 
> Here's an updated version.
> 
> Changes:
> 
> - Deallocate the nation's city_names array.  I wouldn't call it "nice", 
> but it should get the job done.
> 
> - Did some cleanup to the code I added to server/ruleset.c.  In 
> particular, I changed some *name to name[0], some 0 to '\0', made the 
> "handled" var a bool, added a comment or two, and fixed the spacing in 
> one place.
> 
> - Changed a strcmp to mystrcasecmp.  Oops!
> 
> Hmm, I think that's all I did, actually.
> 
> On further reflection, changing char* name -> char name[MAX_LEN_NAME] in 
> the city_name struct wouldn't be so bad; it would make some things a 
> little simpler.  But if MAX_LEN_NAME were ever increased (it's only 32 
> now), the increased overhead would potentially become significant.  So 
> I'm still for keeping it dynamic.
> 
> 
> I have *not* included the data patch or the demo patch here.  The three 
> patches are:
> 
> (1) code patch (included), needed to get things to work at all.  (Most 
> recent: version 10)
> 
> (2) data patch (included yesterday), reverts the changes made for 
> "natural city names" (i.e. separate city lists for each terrain), but 
> adds no new terrain information.  (Most recent: version 6, probably final)
> 
> (3) demo patch (included previously), adds demo terrain information to 
> the American nation's cities.
> 
> Parts 1 and 2 are necessary for things to work at all, and should be 
> applied together (but are kept separate to make things easy for 
> reviewers).  Part 3 should not be applied to the repository (it's pretty 
> inaccurate), but is useful for testing the patch.
> 
> jason

> + * the bonus if the city is close to the terrain.  Both of these entries
> + * may hold a value of 0 (no preference), 1 (city likes the terrain), or -1
> + * (city doesn't like the terrain).

I would still like an enum or a 
 typedef int ternary;
to be used here:
> +     int river;
> +     int terrain[T_COUNT];   

> +    value = (float)value / 1.4;
> +    value = (float)value * 1.4;
> +      value = (float)value / 1.4;
> +      value = (float)value * 1.4;

A 
  const float FACTOR=1.4
? And a comment what it affects.

> +static char *search_for_city_name(int x, int y, struct city_name *city_names)
> +{

> +  int choice, best_value = -1;

If I see best_value = -1 does this means that
evaluate_city_name_priority only returns values >=0?

> +  for (choice = 0; city_names[choice].name; choice++) {
> +    if (!game_find_city_by_name(city_names[choice].name)) {
> +      int value = evaluate_city_name_priority(x, y, &city_names[choice],
> +                                           choice);
> +

> +      if (best_value == -1 || value < best_value) {

Why can't better values be larger. This way we can name it
continuously fitness and not priority or value.

> +  name = search_for_city_name(x, y, nation->city_names);
> +  if (name)
> +    return name;

Missing {}s

> +  name = search_for_city_name(x, y, misc_city_names);
> +  if (name)
> +    return name;

Same.

> +  /* This had better be impossible! */
> +  assert(0);
>    return _("A poorly-named city");

Since this really shouldn't happend I have no problem with a
freelog(LOG_FATAL,...) and an exit(...).

> +#include "nation.h" /* for struct city_name */

indent will place the '/' at column 32.

> +  char **cities;
...
> +  cities = secfile_lookup_str_vec(file, &dim, "%s%s",
> +                               secfile_str1, secfile_str2);

You can merge these.

> +  for (j = 0, value = 1; j < dim; j++, value++) {
> +    /* This whole thing should be handled more succinctly. */
> +    char *name = cities[j], *next;
> +
> +    /*
> +     * Now we wish to determine values for all of the city labels.
> +     * A value of 0 means no preference (which is necessary so that
> +     * the use of this is optional); -1 means the label is negated
> +     * and 1 means it's labelled.  Mostly the parsing just involves
> +     * a lot of ugly string handling...
> +     */

> +    memset(&city_names[j].terrain[0], 0,
> +        T_COUNT * sizeof(city_names[j].terrain[0]));

What about
       memset(city_names[j].terrain,0,sizeof(city_names[j].terrain));

> +    city_names[j].river = 0;
> +
> +    name = strchr(name, '(');
> +    if (name) {
> +      /*
> +       * 0-terminate the original string, then find the
> +       * close-parenthesis so that we can make sure we stop there.
> +       */
> +      name[0] = '\0';
> +      name++;
> +      next = strchr(name, ')');

> +      assert(next);

Such would be an input error and we should print a more verbose msg.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Life is too short for reboots."