[GRASS-dev] [GRASS-SVN] r60703 - in grass/trunk: display/d.vect general/g.gisenv gui/wxpython/animation lib/python/temporal raster/r.colors raster/r.external raster/r.in.bin raster/r.mapcalc raster/r.neighbors raster/r.out.bin raster/r.quant raster/r.resamp.filter raster/r.series raster/r.series.accumulate raster/r.series.interp raster/r.stats.quantile vector/v.colors vector/v.external.out

[Vaclav Petras]

On Mon, Jun 9, 2014 at 11:16 AM, Huidae Cho <grass4u at gmail.com> wrote:

> Interesting, actually this works better I think.
>
> #define G_option_exclusive(...) G__option_exclusive(NULL, __VA_ARGS__,
> NULL)
>
> G_option_exclusive(&opt1, &opt2, &flag1); // no need for the first name
> and last NULL arguments.
>
>
> On Mon, Jun 9, 2014 at 11:13 AM, Huidae Cho <grass4u at gmail.com> wrote:
>
>> Maybe, we can use variadic macros in C99 to remove the first name
>> argument.
>>
>> #define G_option_exclusive(...) G__option_exclusive(NULL, __VA_ARGS__)
>>
>>
Note that C99 is not what GRASS currently requires. Details:

[GRASS-dev] C and C++ standards used by GRASS
http://lists.osgeo.org/pipermail/grass-dev/2014-March/068017.html

> C code is supposed to conform to C89 and POSIX.

(C89 with POSIX extensions)


 On Mon, Jun 9, 2014 at 10:54 AM, Huidae Cho <grass4u at gmail.com> wrote:
>>
>>> I somehow agree with you that my interface can be cryptic and requires
>>> some interpretation because information is spread out. I think we can
>>> define something like:
>>>
>>>         // at most one option from a set
>>>         void G_option_exclusive(const char *name, ...);
>>>
>>>
>>>         // at least one option from a set
>>>         void G_option_required(const char *name, ...);
>>>
>>>
>>>         // if the first option is present, at least one of the other
>>>         //  options must also be present
>>>         void G_option_requires(const char *name, ...);
>>>
>>> Here, actually, name is not needed, but is there only for stdarg. And we
>>> use these functions:
>>>
>>> G_option_exclusive("format", &opt1, &opt2, &flag1, NULL); // No opt1,
>>> opt2, and flag1 at all OR only one of them.
>>> G_option_required("required", &opt1, &opt2, &flag1, NULL); // At least
>>> one of opt1, opt2, and flag1 must be given.
>>> G_option_requires("group1", &opt1, &opt2, NULL); // opt1 and opt2 must
>>> be used together.
>>>
>>> In each function, we can use the state variable in parser.c
>>> (st->first_option and st->first_flag) to find defined options and flags:
>>>
>>> opt = &st->first_option;
>>> while(opt){
>>>   struct Option *x;
>>>   va_start(ap, name);
>>>   while((x = va_arg(ap, struct Option *))){
>>>     if (x == opt) {
>>>       ...
>>>       break;
>>>     }
>>>   }
>>>   va_end(ap);
>>>   opt = opt->next_opt;
>>> }
>>>
>>> flag = &st->first_flag;
>>> while(flag){
>>>   similar code here...
>>>  }
>>>
>>> On Mon, Jun 9, 2014 at 5:34 AM, Glynn Clements <glynn at gclements.plus.com
>>> > wrote:
>>>
>>>>
>>>> Huidae Cho wrote:
>>>>
>>>> > My implementation of the "exclusive" member, which you reverted, can
>>>> handle
>>>> > all these cases, I think. But since you reverted it, I think you
>>>> didn't
>>>> > agree with my interface or implementation?
>>>>
>>>> Not really.
>>>>
>>>> [I also wanted to be able to discuss this starting from a blank slate,
>>>> without the temptation to take any specific approach as a "reference
>>>> point". But that alone wouldn't have been sufficient for reversion.]
>>>>
>>>> Mostly, I find the interface to be cryptic. Information which should
>>>> (IMHO) be in one place is spread out across the individual options.
>>>>
>>>> In order to understand the relationships, anyone reading the code
>>>> needs to examine all of the options, and collate them into groups
>>>> based upon matching group numbers. IOW, mentally replicate the
>>>> (non-trivial) algorithms which form the bulk of r60713. The fact that
>>>> this was deemed to require several examples (with comments) tends to
>>>> suggest that it isn't particularly intuitive.
>>>>
>>>> Apart from that, declaring "rules" would make the transition more
>>>> straightforward, as it mirrors the structure of the existing code
>>>> (i.e. the "if (expression) error();" statements).
>>>>
>>>> By itself, that wouldn't be sufficient reason to prefer a rule-based
>>>> approach if it is considered to be inferior on its own merits. But
>>>> given the extent of the changes, it might be enough to tip the balance
>>>> in the event that the choice between different approaches amounted to
>>>> a toss-up.
>>>>
>>>> > IMO, passing option/flag names to G_option_required() has its own
>>>> > disadvantage because changing option/flag names takes more effort. If
>>>> you
>>>> > have valid reasoning behind adding functions rather than adding a
>>>> member,
>>>> > just like I did, I would prefer to pass *Option and *Flag pointers to
>>>> those
>>>> > functions. But I guess it can be a little tricky to pass two different
>>>> > types of pointers.
>>>>
>>>> Indeed. I originally considered passing the struct pointers before
>>>> realising that approach wouldn't allow both flags and options to be
>>>> used interchangeably. At least, not without changing the structures to
>>>> carry some kind of type signature, although that wouldn't be
>>>> particularly difficult, given that both structures have to be created
>>>> via the appropriate functions.
>>>>
>>>> This issue can be mitigated somewhat by using opt->name in the call
>>>> rather than a string literal[1]. We'd still need to use a literal for
>>>> flags, though.
>>>>
>>>> [1] If we do that, modules which use "opt1", "opt2", etc for the
>>>> variable names should be changed to use meaningful names. Actually,
>>>> that should be done regardless.
>>>>
>>>> --
>>>> Glynn Clements <glynn at gclements.plus.com>
>>>>
>>>
>>>
>>
>
> _______________________________________________
> grass-dev mailing list
> grass-dev at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/grass-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/grass-dev/attachments/20140609/05701a85/attachment.html>