[Freeciv-Dev] Re: (PR#7418) memory usage for compiling

["Raimar Falke" <i-freeciv-lists@xxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=7418 >

On Sun, Feb 15, 2004 at 05:44:28PM -0800, Jason Short wrote:
> 
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=7418 >
> 
> Jason Short wrote:
> > <URL: http://rt.freeciv.org/Ticket/Display.html?id=7418 >
> > 
> >>[i-freeciv-lists@xxxxxxxxxxxxx - Fri Feb 13 10:31:07 2004]:
> > 
> > 
> >>>The former should be quite possible; it requires changes to
> >>
> >>generate.py.
> >>
> >>>  You'll have to take it up with Raimar.
> >>
> >>What idea(s) do you have?
> > 
> > None.  But based on the changes I see in each diff I'd say there is a
> > lot of redundancy.
> 
> There are a switch statements that are unnecessary.  With a single array 
> definition these switches can be replaced by a single array lookup.  For 
> instance
> 
>    void *get_packet_from_connection_helper(pc, type)
>    {
>      return (packet_data[type].helper)(pc, type);
>    }
> 
>    void *get_packet_name(type)
>    {
>      return packet_data[type].name;
>    }

Advantage of switch here is that it is save (type==-1) and allowes
sparse enums. Note that internally the switch will be compiled to
something very similar to the above. It is quite possible that the
definition of the packet_data struct takes the same amount of memory
at compile time.

> Then we have code like:
> 
>    switch(pc->phs.variant[PACKET_CITY_NAME_SUGGESTION_INFO]) {
>      case 100: return send_packet_city_name_suggestion_info_100(pc,
>                                                         packet);
>      default: die("unknown variant"); return -1;
>    }
> 
> do we really want to die here?

Yes. When this code is reached the variants _HAVE_ be initialized to a
valid value. If this isn't the case the generator, the compiler or
your hardware is broken.

> Does this mean the client can kill the server?

No.

> Why is a switch even needed?
> Question: why do packets have different variants?

For different sets of capabilities.

static void ensure_valid_variant_packet_player_info(struct connection *pc)
{
...
  if (FALSE) {
  } else
      if ((has_capability("union", pc->capability)
           && has_capability("union", our_capability))) {
    variant = 100;
  } else
      if (!
          (has_capability("union", pc->capability)
           && has_capability("union", our_capability))) {
    variant = 101;
  } else {
    die("unknown variant");
  }
  pc->phs.variant[PACKET_PLAYER_INFO] = variant;
}

> 100 versus 101?

The memory may end up being 0 by chance (uninitialized). So starting
at 0 is a bad idea.

> How come they have to have entirely different functions to handle
> them?

Different sets of capabilities have different field sets. Different
field sets mean different delta header bitmap. And this means
different bit numbers for the various fields.

It would be possible to merge these variants in one function but you
would end up with code like:

  if (BV_ISSET(fields, variant==100?4:5)) {
    variant==100?dio_get_uint8(&din, (int *) &real_packet->veteran):
        dio_get_bool8(&din, (int *) &real_packet->veteran);
  }

but this is harder to generate and harder to read.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "That's fundamental game play!  My main enemy is *ALWAYS* fighting 
  a 4-front war.  I make sure of it!"
    -- Tony Stuckey, freeciv-dev