Re: [PATCH] Choose syntax

[Roland Illig <roland illig gmx de>]

Leonard den Ottolander wrote:
> Hi Roland,
>
> On Mon, 2005-07-11 at 08:33, Roland Illig wrote:
>
> > > +exec_syntax_dialog (const char **names) {
> >
> > This function must know the size of the ''names'' buffer. I suggest a 
> > second parameter: size_t names_size.
>
>
> The problem is that we don't know the length of names before the
> function finishes. I work around this by always supplying a sufficiently
> large list of MAX_SYNTAX_FILES items. Making the length configurable
> seems not to make much sense. The best approach of course would be to
> dynamically grow the list, but I haven't yet succeeded in doing that.
> IIANM it requires the passing of a char** by reference, ie a char***.
> The code I came up with so far kept segfaulting ;-) .

I think I din't say clear enough what I meant (my fault): 
exec_syntax_dialog should not use the constant MAX_SYNTAX_FILES, but 
some parameter names_size. That makes the relation between them tight 
closer. Similar examples using this style are: g_strlcpy(dest, src, 
destsize), g_snprintf(dest, destsize, ...) and some others.

> > > +	    ! strcmp (names[i], option_syntax_type))
> >
> > don't use !strcmp(...). This function does not return a boolean, but a 
> > "comparison result", which should be checked using the relational 
> > operators (==, <=, >=, <, >).
>
> I know. But as I am only interested in equality of strings ('==' returns
> 0, !'==' 1) strcmp suffices for what I want to do.

I meant: _use_ strcmp, but don't use boolean operators with it. That is:

	if (strcmp (names[i], option_syntax_type) == 0) ...

Don't ever use boolean operators if you don't intend them to _mean_ 
logic manipulation.

In HEAD, I placed a nice macro into src/util.h:

/* usage: str_cmp ("foo", !=, "bar") */
#define str_cmp(a,rel,b) (strcmp ((a), (b)) rel 0)

To me it looks quite good, although it does not follow the usual C syntax.

> > > @@ -272,6 +272,13 @@ menu_options (void)
> > > {
> > >     edit_options_dialog ();
> > > }
> > > +
> > > +static void
> > > +menu_syntax (void)
> > > +{
> > > +    syntax_dialog ();
> > > +}
> > > +
> >
> > Why this extra function?
>
>
> Again following common practice of wrapping menu functions before
> calling the actual dialog function.

cvs diff -r1.19 -r1.20 edit/editmenu.c

It's like I expected it. That was not "common practice" but superfluous 
from the beginning, hidden inside a whole bunch of functions where the 
wrapper was actually necessary. But not in that one case. So both 
unnecessary wrappers (menu_options and menu_syntax) should be removed.

Roland