search

att / ast

AST - AT&T Software Technology
Eclipse Public License 1.0
556 stars 152 forks source link

Remove legacy hash library types and functions #1072

Closed krader1961 closed 5 years ago

krader1961 commented 5 years ago

While doing some cleanups of the name/value code I noticed this block of code in src/cmd/ksh93/include/nval.h:

     // For compatibility with old hash library.
     #define Hashtab_t Dt_t
     #define HASH_BUCKET 1
     #define HASH_NOSCOPE 2
     #define HASH_SCOPE 4
     #define hashscope(x) dtvnext(x)

We have already made many changes that make it impossible for existing binary ksh plugins to work with a ksh built from the current project source. So there is no reason to maintain backward compatibility functions like hashscope() or maintain unused defines like Hashtab_t and hashscope.

TBD is what to do with HASH_BUCKET and the other two symbols above. They are used in calls to nv_search() and combined with other NV_* symbols. Those HASH_* symbols correspond to NV_RDONLY, NV_INTEGER, and NV_LTOU. But those NV_* symbols aren't actually legal for nv_search() AFAICT. The aliasing here is both deliberate and confusing (i.e., a potential source of bugs) and should be eliminated.

krader1961 commented 5 years ago

Looking at nv_search() it is clear the problem is not the HASH_* symbols in my previous comment. It is the use of NV_ADD in the calls to nv_search(). If NV_ADD were redefined it could be an alias for one of the HASH_* symbols which would cause problems. This is another example where the authors of this code relied on their institutional knowledge to know which symbols can be safely combined in bit masks that fit in an int type. I think it is better to keep these name spaces distinct from each other.

krader1961 commented 5 years ago

Also note statements like this from src/cmd/ksh93/sh/arith.c:

(mp = nv_search(cp, root, flags & ~(NV_ADD)))

Given that nv_search() treats several low numbered bits in that value specially how could that statement possibly be correct?

krader1961 commented 5 years ago

The problem is not the use of NV_ADD in calls to nv_search() as I implied in my previous comment three days ago. The problem is mixing legacy hash table symbols (e.g., HASH_BUCKET) with the modern CDT symbols such as NV_NOSCOPE. This should have been addressed by the legacy AST team long ago. Specifically when the hash subsystem was removed. There is no justifiable reason for the core ksh code to use those legacy symbols.