Am 30.01.2020 um 21:33 schrieb Wolfgang Denk:
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);
Hm, U-Boot's 'hsearch_r' does not conform to this 'standard' since
December 2012, see these 2 commits from 2012 and 2019:
https://github.com/u-boot/u-boot/commit/c4e0057fa78ebb524b9241ad7245fcd1074ba414
https://github.com/u-boot/u-boot/commit/3f0d6807459bb22431e5bc19e597c1786b3d1ce6
Note I say 'standard' (with quotation marks) because this function seems
to only be a GNU extension, according to that man page. Nevertheless, it
does seem to have been adopted by *BSD, even if it hasn't made it to
opengroups.org (the reference I use when implementing 'standard' calls
for lwIP).
Obviously, my comments have no real relation to the intention of the
patch to 'clean up' things. I do think the current situation could be
improved (e.g. regarding constness), but looking at the nonchalant way
such a 'standard' function has been change nonstandard, I think this
should be a change we actively vote for (and the above 2 patches did not
seem to take this into account).
Regards,
Simon
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
