| Message ID | 1323264605-13541-3-git-send-email-gerlando.falauto@keymile.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Wolfgang Denk |
| Headers | show |
Hi Gerlando, On Wed, Dec 7, 2011 at 5:30 AM, Gerlando Falauto <gerlando.falauto@keymile.com> wrote: > Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com> Tested-by: Simon Glass <sjg@chromium.org> > --- > common/cmd_nvedit.c | 2 +- > include/search.h | 6 ++++-- > lib/hashtable.c | 18 ++++++++++++------ > 3 files changed, 17 insertions(+), 9 deletions(-) > > diff --git a/include/search.h b/include/search.h > index 2a59e03..7ad4261 100644 > --- a/include/search.h > +++ b/include/search.h > @@ -142,7 +142,8 @@ int hcreate_r(size_t nel, struct hsearch_data *htab) > * be freed and the local static variable can be marked as not used. > */ > > -void hdestroy_r(struct hsearch_data *htab) > +void hdestroy_r(struct hsearch_data *htab, > + int(*apply)(const char *, const char *, const char *, int))\ You could just use 'apply_cb apply' for that param I think. > { > int i; > Regards, Simon
On 12/07/2011 11:02 PM, Simon Glass wrote: >> diff --git a/include/search.h b/include/search.h >> index 2a59e03..7ad4261 100644 >> --- a/include/search.h >> +++ b/include/search.h >> @@ -142,7 +142,8 @@ int hcreate_r(size_t nel, struct hsearch_data *htab) >> * be freed and the local static variable can be marked as not used. >> */ >> >> -void hdestroy_r(struct hsearch_data *htab) >> +void hdestroy_r(struct hsearch_data *htab, >> + int(*apply)(const char *, const char *, const char *, int))\ > > You could just use 'apply_cb apply' for that param I think. Absolutely. I introduced the typedef at a later stage and forgot to update it there too. Good catch, thanks! Best, Gerlando
Dear Gerlando Falauto, In message <4EE5CA4A.8050601@keymile.com> you wrote: > > > You could just use 'apply_cb apply' for that param I think. > > Absolutely. I introduced the typedef at a later stage and forgot to > update it there too. Good catch, thanks! Run checkpatch, and mind the CodingStyle section about typedefs! Best regards, Wolfgang Denk
On 12/12/2011 02:08 PM, Wolfgang Denk wrote: > Dear Gerlando Falauto, > > In message<4EE5CA4A.8050601@keymile.com> you wrote: >> >>> You could just use 'apply_cb apply' for that param I think. >> >> Absolutely. I introduced the typedef at a later stage and forgot to >> update it there too. Good catch, thanks! > > Run checkpatch, and mind the CodingStyle section about typedefs! I did run checkpatch, it didn't say a word about this. What is your recommendation about typedef'ining a function pointer? I immediately suspected that using a callback function might sound like heresy, so if you have any better suggestion please put it forward before I rework this changeset for the fifth time... please. I just think having the whole, expanded signature as the type for a function-pointer parameter or local variable would just make things too long and redundant. Just off the top of my head: perhaps a struct with a single function pointer element might look better than a typedef? Thanks, Gerlando Falauto
Dear Gerlando Falauto, In message <4EE60729.4020401@keymile.com> you wrote: > > > Run checkpatch, and mind the CodingStyle section about typedefs! > > I did run checkpatch, it didn't say a word about this. If you add new tyedef's, it will complain. > What is your recommendation about typedef'ining a function pointer? > I immediately suspected that using a callback function might sound like > heresy, so if you have any better suggestion please put it forward > before I rework this changeset for the fifth time... please. Sorry, I'm in the middle of -rc1 release work and testing, and also right in the final steps of releasing the next version of our ELDK (v5.1). Plus a few other tasks, including trips to customers. I don't have time for a thorough review now. I understand this is very bad for you, but I cannot change it. > I just think having the whole, expanded signature as the type for a > function-pointer parameter or local variable would just make things too > long and redundant. Hm... probbaly. > Just off the top of my head: perhaps a struct with a single function > pointer element might look better than a typedef? I'm not convinced. I'd have to see the code, and compare. But to do so, I'd need a bit of time... Best regards, Wolfgang Denk
diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c index a31d413..871b3b1 100644 --- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c @@ -377,7 +377,7 @@ int _do_env_set(int flag, int argc, char * const argv[]) /* Delete only ? */ if (argc < 3 || argv[2] == NULL) { - int rc = hdelete_r(name, &env_htab); + int rc = hdelete_r(name, &env_htab, NULL); return !rc; } diff --git a/include/search.h b/include/search.h index 2a59e03..7ad4261 100644 --- a/include/search.h +++ b/include/search.h @@ -70,7 +70,8 @@ struct hsearch_data { extern int hcreate_r(size_t __nel, struct hsearch_data *__htab); /* Destroy current internal hashing table. */ -extern void hdestroy_r(struct hsearch_data *__htab); +extern void hdestroy_r(struct hsearch_data *__htab, + apply_cb apply); /* * Search for entry matching ITEM.key in internal hash table. If @@ -95,7 +96,8 @@ extern int hstrstr_r(const char *__match, int __last_idx, ENTRY ** __retval, struct hsearch_data *__htab); /* Search and delete entry matching ITEM.key in internal hash table. */ -extern int hdelete_r(const char *__key, struct hsearch_data *__htab); +extern int hdelete_r(const char *__key, struct hsearch_data *__htab, + apply_cb apply); extern ssize_t hexport_r(struct hsearch_data *__htab, const char __sep, char **__resp, size_t __size, diff --git a/lib/hashtable.c b/lib/hashtable.c index 75b9b07..87adc01 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -142,7 +142,8 @@ int hcreate_r(size_t nel, struct hsearch_data *htab) * be freed and the local static variable can be marked as not used. */ -void hdestroy_r(struct hsearch_data *htab) +void hdestroy_r(struct hsearch_data *htab, + int(*apply)(const char *, const char *, const char *, int)) { int i; @@ -156,7 +157,10 @@ void hdestroy_r(struct hsearch_data *htab) for (i = 1; i <= htab->size; ++i) { if (htab->table[i].used > 0) { ENTRY *ep = &htab->table[i].entry; - + if (apply != NULL) { + /* deletion is always forced */ + apply(ep->key, ep->data, NULL, H_FORCE); + } free((void *)ep->key); free(ep->data); } @@ -401,7 +405,8 @@ int hsearch_r(ENTRY item, ACTION action, ENTRY ** retval, * do that. */ -int hdelete_r(const char *key, struct hsearch_data *htab) +int hdelete_r(const char *key, struct hsearch_data *htab, + int(*apply)(const char *, const char *, const char *, int)) { ENTRY e, *ep; int idx; @@ -417,7 +422,8 @@ int hdelete_r(const char *key, struct hsearch_data *htab) /* free used ENTRY */ debug("hdelete: DELETING key \"%s\"\n", key); - + if (apply != NULL) + apply(ep->key, ep->data, NULL, H_FORCE); free((void *)ep->key); free(ep->data); htab->table[idx].used = -1; @@ -681,7 +687,7 @@ int himport_r(struct hsearch_data *htab, debug("Destroy Hash Table: %p table = %p\n", htab, htab->table); if (htab->table) - hdestroy_r(htab); + hdestroy_r(htab, apply); } /* @@ -747,7 +753,7 @@ int himport_r(struct hsearch_data *htab, if (!process_var(name, nvars, vars)) continue; - if (hdelete_r(name, htab) == 0) + if (hdelete_r(name, htab, apply) == 0) debug("DELETE ERROR ##############################\n"); continue;
Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com> --- common/cmd_nvedit.c | 2 +- include/search.h | 6 ++++-- lib/hashtable.c | 18 ++++++++++++------ 3 files changed, 17 insertions(+), 9 deletions(-)