Dear James, In message <0102016eac3ac1a7-8a163dd4-aa1a-4331-a266-e9f461a07db8-000...@eu-west-1.amazonses.com> you wrote: > > As I said in my commit log comment, there are two key arguments against > this: > > - The fact that the 'data' member of 'struct env_entry' is a 'char *' is > really inconvenient because this is a read-only function where most of > the callers should be using 'const char *' pointers, and having to cast > away the constness isn't good practice and makes the calling code less > readable.
So the 'data' member of 'struct env_entry' should be a "const char *", but that does not mean you have to change the interface of hsearch_r() ?? > - As you can see from the calling code I've had to tidy up, the callers > were very inconsistent about whether they bothered to initialise any > fields other than 'key' and 'value', so if you ever wanted to extend the > interface to check other parameters you'd have to go around and fix them > all up anyway to avoid unpredictable behaviour. Well, is is good practice to always initialize the complete sruct. Where this is missing, the code should be fixed. > Given that only 'key' and 'value' are used at the moment I think my > change is preferable because it makes it explicit what is being used and > avoids any nasty surprises you might get if you changed hsearch_r() > without changing all the callers. If you anticipate wanting to match on > other fields, it might be better to define an alternative query > structure using 'const char *' pointers for key and value, then extend > that, but I would argue that that's something you could do at the point > you find it is needed rather than now. You miss the point that hsearch_r() actually a standard function, see "man 3 hsearch_r": HSEARCH(3) Linux Programmer's Manual HSEARCH(3) NAME hcreate, hdestroy, hsearch, hcreate_r, hdestroy_r, hsearch_r - hash table management SYNOPSIS #include <search.h> int hcreate(size_t nel); ENTRY *hsearch(ENTRY item, ACTION action); void hdestroy(void); #define _GNU_SOURCE /* See feature_test_macros(7) */ #include <search.h> int hcreate_r(size_t nel, struct hsearch_data *htab); int hsearch_r(ENTRY item, ACTION action, ENTRY **retval, struct hsearch_data *htab); void hdestroy_r(struct hsearch_data *htab); I object against changing standard interfaces. I also dislike the seocnd part of the patch. First, this is unrelated to the hsearch_r changes, so it should have been a separate commit anyway. But renaming _do_env_set() into do_interactive_env_set() makes absolutely no sense. It is called in many places from code, which hav nothing to do with any interactive mode. Also, I cannot see what you win by splitting two actions that belong together. I recommend dropping this patch. Naked-by: Wolfgang Denk <w...@denx.de> Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de "Engineering without management is art." - Jeff Johnson