[Freeciv-Dev] Re: (PR#6585) Delta version 10

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

On Sun, Nov 16, 2003 at 11:28:03AM -0800, Mike Kaufman wrote:
> 
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=6585 >
> 
> Some comments:
> 
> ############################################################
> DOC/HACKING: give [order of] files to which you should direct attention to
>  learn about the delta patch. (eg. "Look at common/packets.def first...")
> 
>  Also there should be a step by step of what it takes to add a new packet
>  even if it's just:
>    1. edit common/packets.def
>    2. run  common/generate_packets.py
> 
>  But we should also explain if autoconf/automake/configure/etc need to be 
>  run afterward.

Done.

>  Also, The HINT packets, and how they're used is not explained to my
>  satisfaction. What sends these packets? Why are they needed for
>  compression?

Have you read the section in doc/HACKING?

> #######################################################
> packets.def:
> 
>  Packets:
>  ========
>     
>   They just start and end with a line only containing "end". Header
>   line is: "<struct-name>;<packet-type>;<flags>". If packet-type isn't
>   given the packet type is given as parameter. Flags is a comma
>   seperated list of:
> 
> // this is extremely unclear, better would be:
> 
>   A packet definition starts with and header line, contains variable
>   declarations, and ends with a single line containing the word "end"
>   
>   PACKET_<PACKET_NAME>=<packet num>; [<packet flags>]
>     <type1> <field1>; [<field flags>]
>     <type2> <field2>; [<field flags>]
>     ...
>   end
> 
> // should have a subsection here labeled:
> 
>    Header line:
>    ------------
> 
> // now give exact syntax for the header line. in none of the packets I
> // see a <struct-name> so I have no idea what the hell it is or does.
> // include the purpose and convention of the numbering...
> 
> // this should be a subsubsection.
>    Packet flags:
>    -------------
> 
>    you really need to give a skeleton example here.
> 
>      // "pre", "post" should have separate lines.
> 
>      (packet_req_join_game). Sadly this also disables the use of 0 as
>      the default value.
>     // no packet by that name anymore
> 
>      no-handle: don't generate handle_* prototypes
>     // explain the consequences here.
> 
>      add-cap: only transfer this field if the given capability is
>      available at runtime.
> 
>      remove-cap: don't transfer this field if the given capability is
>      available at runtime.
> 
>      // I take these take arguments somehow. give an example.
> 
>      // nothing for lsend, dsend?
> 
>      // "sc" and "cs" not even mentioned.

Done.

>   # typedefs for arrays/structs
>   MEMORY should be changed to BYTE

I disagree. This is not about _one_ 8 bit item but an amount of binary
data divided in 8 bit pieces. RAW and BLOB are good alternatives for
MEMORY.

> // There should be a comment before the packets begin giving a list of the 
> // packet groupings to come and the comment should also give the number of
> // the last packet so I don't have to strain when adding a new one.

Done.

> // for PACKET_AUTHENTICATION_REPLY you have: cs,handle-per-conn,no-handle
> // but if there's no prototype, why do we need handle-per-conn?

It is not needed.

> // for PACKET_CITY_NAME_SUGGESTION_REQ, the comment should go above both of
> // those packets

Done.

> 
>   # _PLACE_STRUCTURAL: index to sship->structure[]
>   # others: new value for sship->fuel etc; should be just
>   #     one more than current value of ship->fuel etc
>   #     (used to avoid possible problems if we send duplicate
>   #     packets when client auto-builds?)
> 
> // I don't understand...

This comment isn't from me. But I fixed it.

> PACKET_RULESET_TERRAIN=101;sc
>   TERRAIN id;
> 
>   bitvector(bv_terrain_flags) flags;

> // bitvector type is not mentioned above?

I assume you mean with above the type declartions "type A b(c)". These
are only aliases. You can do without them. As these bitvectors are
only used once I didn't add an alias for them. -> not changed.

> ############################################################
> generate_packets.py
> 
> should output (on terminal) what files are being generated when it is 
> being run. (perhaps with a -v flag)

Producing output if no problem is encountered is not Unixish.

I have added a "-v" verbose mode.

> // unintelligible. need more comments.

Please be more specific.

> ############################################################
> packets_gen.h
> 
> // missing packet numbers, wtf? where's 77, 82, 109, etc? and what's up 
> // with 134?

I renumbered all packets.

> // also, freeze_hint and thaw_hint should be packets 2 and 3 or 4 and 5
> // if they're critical packets (though I'm reserving judgement.)

Done.

> ############################################################
> packets_gen.c
> 
> // I still would appreciate function comment headers even if they're empty

And the purpose is? Adding 1000 extra lines?

> // somewhere, the way you handle variants is going to have to be explained...

Done.

> ############################################################
> srv_main.c
> 
> // there needs to be a big honking comment where you #include srv_main_gen.c
> // ditto on civclient_gen.c

Done.

> ###########################################################
> I have NO idea how on earth to actually add a packet if I need to:
> take for example the new effects in packets.c:

This is explained in README.delta. Short version: you have no problem
if you use the existing types (all ints, all enums, bitstring,
bitvector and so on). If you want to use a struct you have to write
code which gets and puts this struct. You also have to write code
which compares two structs instances. If you don't want to do this you
can alway split the struct. I did this for the struct map_position in
the goto route packet. Instead of 
  mappos(struct map_positon) steps[some_number];
there is now a
  COORD x[some_number];
  COORD y[some_number];
You replace the array of structs with a list of arrays.

For the effect I advise you to the first variant. I have also did this
for the current struct impr_effect.

It may turn out that you need a special copy semantic (instead of the
simple struct assignment). This is needed for the worklist for
example. This is currently special-cased for the worklist in the
generator. This can be made more general but the struct user has to
provide a copy function than.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Python is executable pseudocode. Perl is executable line noise"
    -- Bruce Eckel

delta10.diff.bz2
Description: Binary data