Option reorganisation, Part IV.

Option reorganisation, Part IV.

[Zoltan Hidvegi]

Here is the next step in using hashtable for options.  This depends on
Zefram's previous rewite in article 2612.

After this patch the optns array is no longer used.  Loops over this array
are replaced with scanhashtable and scanmatchtable calls.

After this patch it'll be quite easy to add new options dynamically.
This patch also decreases the memory used by zsh a little bit.

Only init.c, main.c and options.c needs to be recompiled for this patch, so
you can continue using your old modules.

Zoltan


begin 644 opt4.patch.gz
M'XL(`*11Q#(``^T\:W?:2+*?F5_1]NZ,P8$$\"N)D^S!6+%98_`%G'&<9#(R
M"*,Q2%Q))'$2[V^_5?V0^B5,)C.S>^Y9[ZQCJ:NJN^M=I98V-S=)/QH^"N>)
M'P;QPV&A]N3)[J-:_5%]E]1VG^[4GE;W"EL/:_"_^@^52B4/>H_4ZT^WJT]W
M=G_85'_PFFP]+N]L$WI)2)Q$BV%"@$K@SCSR!6[AS[$;3SKAR".!]RG9+Q0>
M;=*_B!^0"0R1X<2%/S<?<7"XC,@F4`!0A*4P(S=Q$62-@O@!FR7D(&S=)%C,
MKKR(4;K;AU\_L,D'[M74XT")>\5'`&\P\<C0#<+`'[K3E`JN/:$HE!+,^;>1
M-_8#CW3/!N^;_>-"H5@CSYX1Y_2\W1@X>*ND09V84"<FE`EDPER:0)<(1?<@
M`S;:;0#D2_S*%_&53?.54RII.`?=\U['85@9=(DJ!(KV,8&_*G^U:,=3]SK>
MYS<6`0KE"UP5&'J23N5.?3<F"9UPS'G0:O09L8*L)#8=8>3OR&+_3U<725%L
M>B)IB$5!,M6P:8:D$W^N2MBL?V>W7-O;$N8O4^ET.S(A6`CYB?PKHZ_/"O"X
M'1U8:+JZ<;[G0O73=E42K1^3R)MZ'UP0?!(2;[:8NG2`"EMAZYG3;#7:0."Q
M3"">A(OI"#3X`VC(E4=B+R%7MYR05RPQ0O!?G`#AH6040D45)?>XEH\7TRE3
MDU3K%D'L7P?>B`$S?4?0V/.(>Q5^X*!W9`CNF*EQ_(:NO'7IO"//T\F^=,[;
M[3*IWI7%C75W.O4^S<,H62^30D$=^>C>QE,W3N91.)M3`',\";U@9.+.YW![
MXL-P=&N.+I)PR)",^U/`L2+,O&!A'4!>C?PHM@[.W<B=W7BW2T9QBQ,ZS%5?
M!UK$$\L.823R9L#\%-\<CA<SS\"\<D=S-TF\*$@GY2KZ5;$%!26XG@C&K(3@
M>7/&7VXB\M@U>"0O&[V75N0.O>%P:FXD'GG#26A=E,+%X0@=W@?7(B/0Z-B;
M^L&-96@:7H'S3<FKNQB"2DZ]Q*->W8MU_HMA/_@81J;TAF$4>4-3T?A],`IS
M*)[\M@AN?$_2:GW7JO*D&-,PG,=6-N4@_.\B3-B>[D4(P%U<`Z=6(`]DW6EL
M%_RE"@E<^.2;[/$^><,\M8*X"B;OC=+%5"V#*_-N/`T_@C=+HG":HP#C13!$
M3^I&UY^]"+1P-6U.EV<AB6-NC+YV!6XBL-`R8[\X.`H34ZEQ(%Y<Y9NR*@A,
M>8:S49S#`QQ.79]]&/VI4&<+!!L%7F?&5M7&4U]B1P=N@:<;+>;F9K/A>.XR
MEZ,/!R$JA,E`'$.J0^]JZE+O8`Q#V/7'9G"9+.:KJ#A;%_5M=M.T0'OAV)@.
M\D8/B"3^!R\EPQ,&.Q3H#(2RQ&K=AK;>Q!#Q(_?6OD0;.$LOYA',:,4Y4?9%
MM6-VY5\OPH4IO^ERZ>-P<CN7^*=:R#2$Q)<7B:NL);SV@TQT)A.G87"-<_X&
M)F2L=>9>0U2C'DZ8ESKL0[H2!7YP;4&-;JP)!*8<N28^@\0>5'>)T"$1A+W;
MMZXJ8Q#.W&0XR?,)AIR#,!&J;Y%+7DA00&9@/4/JC7CN)^\-LIH([,^_,;<-
MJ<O$RJQY&/N?KA8^[!G%O8*V4B7%.//!G2XL,T7^!W_J77NC)3QFJ>DP+TM@
MP_D.5]5`FNFI_JRJ#\_\P&(I="B&U09FT*1C23@)+;E@-(3DVPU&-!&UC&:)
M@#80YPD?TA?ODVM);:(9F'HDK=$JDW@R!@BZJ!CL=B4YQA,UJ-X##.J1C/Q@
MB5"%%P.[`[>YFC(!3I2HB9;*F'B"F6`\G_IV53`I@J^84G?M8H63NU@*!BFL
M]WF:A0!5L6+(ZKS;J]"-1A-W>*,K%M1W7I+#O_ZQHVP#+/,JC$U-^I1$KLCJ
MY?MB4=KJLQX&U)%=5I)R=F,QB1V=W6JY]F1'M'3^\G*]6'OV;*?T704[DM@M
M?7/)KO9$6HT^);2GK<4-1$<IQ+_#9`(T^;"H_`4E^`>F*EZ42'%MK0C_5E[0
M*AXXD6ZC5+)U"T3EKK;.]-Z!N&&TP[*N%NT/L,Z`Z`CP;H#<`BA0+%(%;2\0
M$)5S<=;M#:@Z9=!&6T#"^+GQNM]N]`=GO>[IF0V1]PNTB1!MT'4ZARJ&VD50
M<<[.`/RX!6B]URH6[RZHX.>#;O/0@&/=!@.R#60-6-:`,&!/G<ZY`9OU)`SX
M3N/4.6SU^@:.U*HPD,X:O<;IB?,Z!TNT(.@/3\147!3*L8G,6AOF=.?]8Y-9
M:KM#Q>@YI]U7CGT6T0<QINDY_?-31X&7FR.%;#_6A*A,#AJ'9XW!P.EU-"*B
M7;(*B<[1L2YNEO06)&RP!;KL`\<Y4T%Y-V6%F8XZK::VV[2UHO#FH-=H.LUF
M6X45W9:<F5#BY*!_Z#2/NPJBW'U1IFD>-@[:SJN&IHMR2T:%/V[TG7:K<Z+!
MB]+1PJYFNWMPX/14>*-S(TT!7J/M#!SJ=IV^%2]MZ5CP6IV?N[U##8VW>M2]
M='L]ISFP0=)JV08L\HD4WNP(6>1"39'`[W^>=TY:CLUC&8TBJX`U0NUN]ZQO
M)Y-FC2N0^9_S[D!GM-Q5RM&VE`KB'(&4%0JBTY2#?$F18>I&6YTY[3LI['>`
M^Q<M55:L$V6S4.?":7;/=&BY-Z42OQA`''$.S3T8+:M\/@@B-N$JG2S+>E^V
MNS\WNYU!KZMJE]'?NL^_O#SO-`<MB"^]HTNGI_H`OG'+]+AOG5UR'VRI_!&Y
MT>^WCCH&?E8W*^Q&#&&L!@[KF!GPA]U!WX#E-5V^V[U,5]@_/]#\>]96LW`$
MO-QQ\_2P;V`H`5W',$*ZTGS+0<(\0_<JEI9<RHUC!@\*8_&K6:_.-AM@&K'+
M:-\1=2J0*\3HPW/-SYAM/1M:_ZRA13NEWU?0)^MTT79,#+4+J.+@XIK.0;NA
MQR2Y/VC,],KIM5ZJ1DJ;AH6E[NKX7&6>VD!<%I09/VA0[UM(T*ZBLDB&X'1?
MJM!RIU&:CU<[N/Q6!W*A!GB!5TX>JM1^M%?`984,V"HDN)K]25W)G'VGI*`.
M;O1ZC=<&`;5/:25R0KD'O^$"_-I9#]:ED-&ZEPH/J6F='K2.SKOG?0,KUT[:
M-CN1&IT2BLBQ$67P^DR3K=K\S&$3VV&[VVRTV1YU&K0C6K`+N]T]:G6R]D"&
M)'=)59YT.T>XVG^"3U1PS.9IAG3:.(*TE<9JPX\J755EJM-&"XJZ7J?5.=(P
M1*]5`^^=&!Y4;;^J\%!T6<-(VI6U\^RTVVF!EU%11)MVJ?G##1"0NL"T;7M_
M\M_IGC8&S6,-/9$=E*R&G>Y`=U'+4C+<FC41,_N\&0\[4'OU6DT:(?4B7VX`
M*XR'(J\'?K=UHK(]ZPHKT%"<F8%1;Q3?XT'.NOW6Q<%Y"]BOF8?1/Y9G1F>!
M6>.K1OO<T='2OK)=2P#Y5:OM'#F'&J+H-5LDQAH>S9X%8WFJPGP`0S<MS&Q)
M2WO$*CTG1LN]:E4DB'3:ZIQ;X$5SV$3H`SLZEI6)MK:),>@>=[7:7FMV*SB]
MIG-QUN@<TF:%AB7*&0W!4KG0OK@MR^TU-<"T2ZX2=6`9#:TL5!OG$G&AHH!W
M"L&F9V&2T4Y?KNO]XY=`@W*B#]%`([6T()-H&%Y`ZKG;%;Y_#+8U.&SI,VIM
M^/LF9C$,?#*D#WV=5-J=MX@'4'L#LZ!56O;W[QM+__Y96ZL4]3:^G0$0I]HT
MV6EH/4BMO2^AG_#XSW#;K8YSV585WFS[9ZK6A^K;>7W0;?0.CQO-$P6//P_(
MRT?9,P%8RGFG[ZB;39\.*&H-/ON@VU?7)IX7*(`7@YZ>M+-=V[EVWM>WS,^7
MX1$S;#CO_]'];.E,IVAH0\Z2=K0?;9(;/$<*7A<FO?)!<6Y%3YZWFUAO7FG`
M,:\$NTP;_C0X*PF^ZOHFMS$>ZM10I,:9J@;<]!3@S"856)3*#2L99>BT8,UC
M]CW/<VS',6M[>^7:8^,\YDN(UX/W,#G9J&Y(][&KSV[?;K"G-D*R:-OD,U2[
M;+XW*61%(O:`U)B4F";!*JNP,")Z:^7L?HW>5P.X-%S'X4K6^2W3AU6U)T_*
M]:KQM.J;-P,IYXQ0Q_?OW)%-7/6=[7)]MRK$Q7%O*6G)]V6/]C0!W?S.[52-
MC53UM>,=%$)];[=<?UP70OB&)4IL_Y/7:7TM8:M6WJYM65E;S59;D6Q+\B85
M'#'.^+I36&(E<UKL:"X[Z,OO2P_G*O+AW<RC5<1:3*]&)\:?+YH3JVCM!@$E
M.2VUDR\`LJ?R2CX@AB7/)+?(*LB<"G<\+2B^?&!-S![,AF.23#SEZ#L]PZ^>
M?^>L^Q#Z(^*.1N*E@??%HLRX,K*K5-K7$(:1YR9>>OI^2O%P1`'M<IHR=(#1
M!X`Y6>X[Z:(0'_;D8RG)*GA&$NY]D607[#/94QGKZRB6A)C'850,0)PT>+8N
MG7VPCV"?(+5"MM\B>S8;O'N(5V42&/BQ_]D+QT6F(*5'RN6;ZKO2/@DJ%2M=
M!$@)9]=4]^@T=XH2\^U+-'0-+F>Z*S,EW7[E!2`C@XOI+=24PN<D@BJFB"1*
M94,:148PW?B]Y*!`';I)$6(UJ.TZ_#^/<$6B;-FK37Q,X"1-6-(QD$3@?40]
M9H`8>:!@%L/K+"X+C=+V0=6?_SRGQN!%^R:8!U7C+5-G`$-Z%B"H+*8<)A^(
M,XY/"%<X)][9U_=5>7'M)1(L7`G8NH4P!ZZO!,R>'#/B8JG&]"-P&[`;"I4+
MY`4IS!+.1)XG=I)+B78/.%1&Z4[Q2=QOP`U#I:@#6,QD%1%>)@QX<*,SHND6
M^5")?!YB;W]89-9+-L.@E*XMJ+R@>HH:MDASX,A+%E'`B=ZE.5X?8B24)NAC
M(:=Q%U/Q9DG,':OJS4AZR(69,4ND(<5YG]DSOGERRS9D"5!98/PYC&Y(N$BH
M<P=;D([BS)"?Z0M<_KBX!A%Q.)L7LYGP\>)Z":<I9'C/Y;?C]MD+93M;$)1W
MK,E$53O`]-UQARJ#)-VT/)D$C#=0D-F"Q"J>0U:+-7*O.ZG6_K^[DW^#-^%7
M2U:K.A9VM63!O\/%:#HF*0-$>%+4'`5-!_:I4\""&/YX\`!/B!661D4!CW^D
MCB6'/&0!WTM?]Y?4)M96LR:KA2AKG,A^U!_C'K)S<VEU3!=-9QP7UW^,]:-Y
M/\9O,:F5UEUYL7B('824/]XT]BA]/\93>NH#IU*)O4V*X_P8'S^K1WLB8N(@
M_+%2K\;Z;(PBFY/E(.0?8-WC\3IY"O\&ZW01!5R!3A'HU5>E%S!R0#;=U%VV
M+3SE^/RYV)Z"_`N1]B3M%%%^L<R6[GL\7R0QS[R@<(!HP'9";_/E9FOYMLCU
M0''-L`01N@P]8A%K'S"^+]S]A:'-5G]NUQZ7M[?WY/J3:D0.G4ND0Z2`#!P5
M+'Q*/'<X(0&(()Y[0S][\QCL`J7J#H=A-$+^@W'@+L6,9),,%U'D`6>T38-"
M>#X]WNHG2`99ZH^\R!ME!W*)3$<YG0O.YZ-'W,@CHQ"G==D;KAE`$0S5#T:8
MR@-%F<[5+5WAKU12&X0^-P!MB$H/R?T_Z=NS6#^)O$JNP5AF0TI,YXM%.@OY
MZ2>RQFLQ"O#NH>QQ>/,33.7K5V$*A7QP+K',;`!&`&)4XK;'ZS!N))6\S"3F
MY2IUGZ6LZH9+`"@3VC;&%HJ6``(*_+[R@_=LO)CV6<O<,#;=Z#H6%^$\9@;B
MQXM`M@^\Y](S2666%I8)??X).ZE*GAK-"-0%=!"D?KV@IPU0"_@FRAB,YU-7
M-&9C$":^*@]6!\X#Q+\FQ`^"`_JH<2.4^LS'5Z"Y3BAL$4>E2/'*FX8?03TD
M.R7%-;H][MUDQ.(:W2*NO<#3[*J(]4P4D(=NUZOE[9U=.0]=Q3@?4,AXZ`99
M;B?%T:KX+W-O9<F=W:%+4_P@7MWK"0'H"Y]Y24Q]\%_7L:+KH-J3DW:`IT!]
M4OV&!FFX"P,B]1`B]P+WD,5;V4E(^=8RJS>")Y.AJ3'TH;D6.__C/46^7>=:
M6HV:F?0PQ9:@E\E]KL#Z08OJ=GFG5A=QNU#`<YY^L*`%0T&@%OA6VV$X)WBP
M@J3/@]F.1$JU0IRBR1D#6GLNG5-`]1NQTRAJ-&*)R3`$`5:S,$1_C4(N9"XA
MR@$$2U>/['WP@%[>I;Q`G[A3VX*-/Q$^\3LWCJ*C2S=DQ]:M2"_5:$5DN:NU
MBJT&8MO>E=,M6>1W::^?JH><G-*(FUOKQ$5+;2/U4)1:9EFMP1K^E3BY3;\L
MDYH"<!]/=T&!,\WL9).R<I-\1&N"ZH?[1-X+$;X9%[Z(-3>7EDY-#L7GBUF6
MCO4'R^QE]:SMLSV19Y*FTCM01+(]B'TJ.0[3X9R:R51;7G_DUTUI,);)*H73
M"D2UXJEPIU900B(ZUR&^0'*"P8EZN8B.9??&8\[?W\DWK6C3RS7&3HYCEFP2
MMZEKR:G6>+FF,REC[IW"$"7^M$:@+?[XEK[\)G7:^-N#._5:>6=K2TZ;#"/3
M;>G>:/6-/8/,P]DB.`@?QJ9A>+.8%Z6V`&B#%(++@A5L"<(_KL0'^T>0P/GL
M93'C3JGD<#5D,6<4_(#F,&G7DFW*Z.-H/339?\8E$<.@P`FH/G`AB%ZS&!<I
MK2HC*L@=$.1N+?7UW[Q@JIQOJN\P8FT$&Y`*D?A-C5V&&SPS6BM^T[[(`U+G
M.=,?QH_E"J+H1Z8>R_FWR;^K]SF>/)PHW]1[\G1G^^GV8_Y-O9WTFWI6R*VG
MVW7[$0Y(0&JU]$S`#]D!!\P\F\>-7I^L_^WOOVP62\^_?KE[\^[79R_^\:_]
MG]X&;Q/R]NW;C;?KZ\Q<Z`-XD;YA.W/0/2G3[U%5299U]9TSIR>>JG><G]LM
M<?R"+X0KR?<NA)T$^'W+L?'IR?9V^<G.CL0GH'06QK&/FDH-.\9(J;Q3+/,%
M-5EJXY01OPG+],#G<T67/LM&AT_"*+!#(`!"'(2@*IX=!LI("O-KX.*I_8VT
M_;0F/_<0]1F&>T^TSU`>;,-;DCQ6W;"0F[1;0O#]YUK)V+0.>R)@ZR4;!W1P
MA*;@6R4[.W2$2T%_NV3ES5*VV.UG=Z=<J^[5TD-0BP#F9%%44F(65?'U;5JA
M7;R3WW"G!1A[MSL=K>2\^LU"[476)6*?:I1>`M<^U2C>W!")@/1J1DE>Q&_A
MU166R`*.'W0OE;AY[F[!/G?Y!QJ%6YJY?B!_O7,'O_59!;^T8WSK4P?=0\=4
MVWI:W<N)<N7=NIQ@CT<T*KS'YYZTOG$Z[T\;%_OJ,(R(!Z0R0DG.G:5#&:DW
M%VV1K!5;*^VS`.6+1X.@"+,PUGK-^$.%EKU,@35WD=:Y'S`!VZALI--02,[9
M=RR=HW/,("^_XHJ`01#G?.^'['L"-(92=DBIT!_&#NNYD_]\IOP?WB[WEG%6
"``"=
`
end

Re: Option reorganisation, Part IV.

[Zefram]

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.

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

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

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

-zefram

Re: Option reorganisation, Part IV.

[Zoltan Hidvegi]

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

Re: Option reorganisation, Part IV.

[Zefram]

Zoltan Hidvegi wrote:
>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.

The point of a hashtable is to make name lookups very quick.
scanhashtable() is used when it is necessary to look at all elements in
a hash table; in this case it is not necessary, as there is another,
easier, way to look at all the options.  And, of course, going through
the hash table elements means that alias entries will also be
encountered, and must be ignored, whereas going through the option
table avoids this.

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

That already applies.  The reason we have to recompile is that we keep
the enum in alphabetical order too.  Anyway, we don't add options very
often.

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

There are other mechanisms than these 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.

Then autoloading an option is equivalen to letting the user create new
options on the fly.  I really don't think we want to do that.

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

I don't see what you mean here.  The option aliases should never be
output; they're just extra names that can be used when specifying
options.  All that is needed of an option alias is a mapping from name
to number, and putting the number directly in the hash table element
achieves this with the minimum of fuss.  There is simply no need to map
from alias name to canonical name.

>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 difference here is that options *are* internally identified by
number, not name.  The system I produced had options operated on by
number internally, with simple table lookups to get the option flags
and canonical name, and used the hashtable only to map user-specified
names to numbers.  Your system uses names internally in some places,
and numbers in other places.

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

That's no excuse!  :-)

The code can be made more portable by having two structures, one being
the normal structure with the union, and the other the same but with
the union members the other way round.  It is possible to initialise
the first member of a union, so by using these two structures you get
the ability to initialise either member of the union.  An
implementation would have to be actively perverse for this to fail.

-zefram

Re: Option reorganisation, Part IV.

[Zoltan Hidvegi]

Zefram wrote:
> The point of a hashtable is to make name lookups very quick.

All right.  Show me any scipt which runs measurably slower after my patch,
and I'll restore using the array.

> scanhashtable() is used when it is necessary to look at all elements in
> a hash table; in this case it is not necessary, as there is another,
> easier, way to look at all the options.  And, of course, going through

Yes, there is but theoretically it is cleaner to use hashtable functions
only.  And just to show how much more complicated the hash-scan code:

        for (i = 0; i < ht->hsize; i++)
            for (hn = ht->nodes[i]; hn; hn = hn->next)
                if (!(hn->flags & flags2))
                    scanfunc(hn, scanflags);

The if condition skipps four option aliases.  The overhead is this four
skipped options (from 108), and for each option one pointer dereference and
assignment, two conditional jump and a function call/return.  At worst this
means a few dozen machine cycles.

With setopt without arguments which prints all options, there is the
overhead to sort the options but that's again unnoticable in interactive
usage and setopt without arguments is used very rarely in scripts.

> >Yes but again 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.
> 
> I don't see what you mean here.  The option aliases should never be
> output; they're just extra names that can be used when specifying
> options.  All that is needed of an option alias is a mapping from name
> to number, and putting the number directly in the hash table element
> achieves this with the minimum of fuss.  There is simply no need to map
> from alias name to canonical name.

I just thought that a form of setopt to print option aliases would be
nice.  But I really do not think that necessary and I'll gladly remove
textual aliases together with this union hack.

Zoltan