Re: Option reorganisation, Part IV.

[Zoltan Hidvegi <hzoli@xxxxxxxxxx>]

Zefram wrote:
> Zoltan Hidvegi wrote:
> >After this patch the optns array is no longer used.  Loops over this array
> >are replaced with scanhashtable and scanmatchtable calls.
> 
> But it *is* used.  It has to be used to insert the initial options into
> the hashtable.  Any use of it after that we get for free, so there's no
> reason not to look up the flags and so on there.  And walking through
> the table is faster than scanhashtable(), for those functions that need
> to look at all the options.

Yes it is faster but not much faster and it is quite rarely needed.  I do
not see why should we use hashtables if we do not use scanhashtable.  If
you want to use the array you might as well stay with the old system adding
binary search to optlookup to make it faster.

Hashtable functions has the advantage that the order of the options does
not matter.  If we add all new options to the end of the enum list we do
not have to recompile everything and we can keep optns list alpabetically
ordered.

> >After this patch it'll be quite easy to add new options dynamically.
> 
> But do we *want* to?  I don't like the idea of options that can't be
> set until the appropriate module has been loaded.  And autoloaded
> options would be silly, and would cause unnecessary module loading.  I
> think options should really affect only the core functionality.
> Anything else can use command line switches or parameters.

I did not want to move zle options to the zle module.  But one might
implement a new module which requires some new options.

I did not think about autoloaded options but even if an option is
autoloaded it can be set without the module where it is used.

> You store aliases in a manner that requires a second name lookup, but
> it's easier and more efficient to store the actual option number.  (And

Yes but againg you do not win much here.  I could store numbers for aliases
but that without optno -> name reverse lookup I'll not be able to print
these aliases.

One more argument: builtins has been stored in a static array before
modules appeared and noone wanted to use array functions on them.  But I
know it is a bad example.

> the way you handle the structures with the union is not strictly
> conforming, and quite likely to break on 64-bit machines using the LP64
> model.)  I don't see the point in doing this.

The struct iparam in hashtable.h is even worse.  And this can be made
safer:

    for (on = optns; on->nam; on++)
        optiontab->addnode(optiontab, on->nam, (Optname) on);

where on is of the same as optns, and similarily for optals.  That would
only assume that the union is alligned the same was as int and char *.

In hashtable.h the code assumes that sizeof(long) == sizeof(void *).
That can be fixed by duplicating the table but that would just waste memory
for nothing (at least noone complained so far).

> You also changed the {z,k}shletters arrays to be of the enum type,
> rather than short.  This is a bad idea.  On 32-bit machines the enum is
> most likely larger -- if we really want the extra speed in table
> lookups we should be more sure of it by using int.  More importantly,
> the enum could be unsigned, but the table contains negative values.
> And there are probably some compilers that would warn about the use of
> a value that is not one of the enumeration constants.  (For these
> reasons, I *never* use an enum as a type.)

I agree here, I'll change that.

Zoltan