Re: [PATCH] Choose syntax

[Leonard den Ottolander <leonard den ottolander nl>]

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 would have written option_syntax_type != NULL.

That's indeed more obvious.

> > +	    ! 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.

> > +	    option_auto_syntax = 1;
> 
> TRUE

> > +	    option_auto_syntax = 0;
> 
> FALSE

> > +	    edit_load_syntax (wedit, 0, 0);
> 
> edit_load_syntax (wedit, NULL, NULL);
> 
> or, for the C purists: edit_load_syntax (wedit, (char **) 0, NULL);

<etc NULL gboolean>

Strictly speaking these changes are more correct, but if you look at the
existing code I've followed existing style. We could fix this globally,
but I'm not going to do it for this patch.

> strcmp(...) != 0

Nah, too much ;) .

> What about the name edit_choose_syntax_dialog, which fits much better to 
> the ones surrounding it?

Or edit_syntax_dialog, yes.

> > @@ -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.

> > +/* This list could be allocated here dynamically as we don't know it's size yet */
> 
> s/could/should/

I'm not entirely sure it "should". The memory overhead of the fixed
length char** is not that big. Using a dynamic list requires us to clean
up on two places in the caller (or use a goto on cancel) and some
changes to the interface of the called function, plus some funky pointer
arithmetic ;) .

This is even more so in case we save the list to a file instead of
recreating it on every invokation of the editor. That would be much more
of an overhead saver but it would require the user to refill the Syntax
list once it's edited. I think that is not as bad an option as it sounds
as the user is usually aware when he has edited the Syntax file. Another
option would be to test the mtime of Syntax on every invokation.

> NULL.

You sound like a broken record ;-) .

> > +#ifndef __SYNTAX_H
> > +#define __SYNTAX_H
> 
> All names starting with an underscore are reserved for the C 
> implementation (that is, the compiler and the standard library). 
> Application programmers shall not use them. If you had written your 
> patch for -HEAD, you would have seen the change.

I think I've read that before. Thanks for pointing it out. This is
clearly a case of me too strictly sticking to existing coding style :) .
You've fixed this in HEAD? I'll fix it for this patch as well.

Leonard.

-- 
mount -t life -o ro /dev/dna /genetic/research