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
