Update of bug #13621 (project freeciv):

                Severity:              3 - Normal => 2 - Minor              
                Priority:              5 - Normal => 3 - Low                
                  Status:                    None => In Progress            

    _______________________________________________________

Follow-up Comment #9:

Please use the bug tracker to post follow up
comments, so that people looking at the tickets
will see the full converstion.

> I will take into account your comments and rework all
> the settingconfigurations. As I see it, there should
> be the following functions (in settings.c?)
>
> bool settings_check(int cmd, union bisval, ...
> bool settings_set(int cmd, union bisval, ...
> bool settings_send(int cmd)

I would adopt the convention that the "class"
is 'setting' (since it is 'struct setting') so
the prefix for all functions is "setting_" and
the first argument is 'struct setting *' (or
with leading const if the function does not
change the internal state of the setting).

Also maybe 'setting_update' would be a better
name since in 'setting_set' the "set" is
repeated and this is kind of ugly. "set" can
also mean a collection of objects in English
so the name might be a little ambiguous. This
is just my personal opinion though.

> - int cmd:
> number of the command in the settings struct

It is probably better to use 'struct setting *',
since one day settings might not be in a static
array. It's really not that much of a problem,
though if you use the "int id" approach you
have to transform 'id' -> 'struct setting' in
each setting_*() function, whereas if you pass
the struct pointer you do not have to reference
the global array in each function (or call a
function that does).

> - union bisval:
> bool/int/string value for the option

Why not 'union setting_value'? This would clearly
indicate what it is used for, and the individual
field names would tell you the underlying types.
I don't think someone seening "bisval" for the
first time would have any idea what it means.

> - char *msgstr:
>   return message by the function (error/success;
>    only displayed if length > 0)
> - int *msglen
>   length of msgstr

Alright, but check if these are always necessary.
Maybe some of the setting_*() won't need them.

> should there also be a function
> 
> struct settings *op = settings_get(int cmd)?

If you take the "int id" approach, though with
the better setting pointer method you would
just change the parsing function to return the
setting pointer.

Maybe if you find that 'int cmd' has an overloaded
meaning as a command id (for 'struct command'),
you can go with the id approach. I do recall some
ugliness in stdinhand where some function returns
an int value that could be either a setting id or
a command id (help_command?); if it is too much
trouble to fix this too then just use ids.


> Some questions:
> 
> Am Thursday 18 June 2009 03:45:29 schrieb Madeline Book:
> > - Sometimes you cmd_reply (= notify_conn) in the
> >   set_command_*() function, and sometimes you
> >   notify_conn in its caller; in other words, what
> >   is the point of passing 'pastr' if you just
> >   cmd_reply inside it anyway.
>
> that is the difference between cmd_reply and notify_conn?
> 
> From the comments to
> cmd_reply I take that it does not duplicates output on the
> console? Should
> this be used for calls from the command line?

cmd_reply prefixes the message with "/<commandname>:",
and echos to the server console if 'caller' is NULL
(i.e. when the server operator enters the command).

To be consistent, just use cmd_reply in all server
command functions ('*_command()') unless you need
to send some special message to connected users
like "Server: Exploding." which the server operator
would already know about in some other way.

Otherwise use freelog() if you just want to print
some information to the console.

Maybe this business of echoing or not echoing to
the console should be examined and cleaned up too,
but that can be in another ticket.

> Should this function(s) be called every time something
> should be printed or at the end one time to print all
> text (especially for the rulesets)?

Ruleset loading should use freelog() to tell the
server operator what is going on, and only send
a message if it succeeds or fails to users (this
is already done by the rulesetdir command I think).
Actually I don't recall that regular freeciv can
gracefully fail loading a ruleset, it just dies.

So any "error messages" from the setting_*()
functions would be only advisory, and should
probably only be echoed to the console with 
freelog(LOG_ERROR, ...).


> > - You replace the fast pointer checks with slow
> >   string comparisons (strcmp(op->name,...)) for
> >   no reason (yes we can debate which is better
> >   but the fact is that both depend on some hard-
> >   coded name, be it either the string name or
> >   the variable name, so you do not gain anything
> >   by changing it).
>
> So the following line tests, if it points to the same
> place in memory? At first I thought that it will be
> true, if the option value is equal to the value of
> aifill (so if aifill=5 and mrg_distance = 5 it would
> be called two times). If it is not the case, I will
> use this faster version!
> 
> if (op->int_value == &game.info.aifill) {

Setting definitons are a bit strange in that the
macros GEN_{INT,BOOL,STRING} take the address of
their 'value' parameters (this is what the pointers
{int,bool,string}_value hold [really they should
be called pint_value, pbool_value, etc.]). Maybe
the fact that "int_value" seems like an integer
value due to its name confused you; it is really
an address (same as '&game.info.aifill' which says
"take the address of 'game.info.aifill', which is
the same as what GEN_INT() does).

By the way, it occurs to me that this way of using
pointers is not really compatible with the union
idea, since the union fields are not pointers.
Maybe then you would need a second union that
would store the "pointer versions". Or perhaps
you would have to only use the union to pass new
setting values around. Still, it looks like there
is no nice way to combine the hard-coded int/
bool/string pointers in the setting struct... :(


> > My suggestion is to factor out into separate
> > functions [by "factor out" I mean take the duplicated
> > code and put it in one place so there is only one
> > copy, not make 3 copies; cf. 2x + xy + 3xz =
> > x(2 + y + 3z)]:
> > - setting value checking with bool return indicating
> >   success and error message buffer for reason:
> >     bool setting_check(const struct setting *op,
> >       int ibval, const char *sval, char *err, int errlen)
> 
> first arg int cmd or struct setting *op?

Up to you. My instinct is 'struct setting *op' (or
'const struct setting *op'), but as mentioned above
some dependent functions might work better with the
id approach. Just don't call it 'cmd' please (since
there are also 'struct command's). Call it
'setting_id' or something obvious like that.

> > Of course if you start to use the above approach
> > and it turns out to be worse than what you already
> > have, feel free to ignore my suggestions. :)
> 
> After I read your comments I thing there is place
> for improvement ... ;-)

Acually I am just working from memory without looking
at the source code nor checking if my proposed "design"
would fit well in the system. Take it with some
reservation as it may end up not being the best
solution (or even basically correct).


> > Really I would have been happy to commit the last
> > version, since if you dig much deeper into this
> > code you will probably end up making a substantial
> > rewrite. It's up to you if you want to continue
> > with this less than exciting stuff or just leave
> > it for later...
> 
> You can commit it but I will check (and perhaps
> rewrite) the settings system and as this is a new
> feature, it can wait.
> 
> I will need some time for the next version of the
> patch ... first learning a little bit git and there
> is also real life ...

Alright, I will not commit the last "ok" patch
since it is not a critical change. You can take
all the time you need to play with different designs
to see what works best, then post here what you
come up with and I will review it again. Or if you
find after a while it is just annoying to rewrite
most of it, we can go back to the last patch and
just close this ticket with that.


----------------------------------------------------------------
また書きすぎてしまった。

    _______________________________________________________

Reply to this item at:

  <http://gna.org/bugs/?13621>

_______________________________________________
  Message sent via/by Gna!
  http://gna.org/


_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to