| Message ID | 20190728140357.137295-32-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | env: common: Remove environment definitions from common.h | expand |
On Sun, Jul 28, 2019 at 9:28 AM Simon Glass <sjg@chromium.org> wrote: > > This typedef does not need to be defined in the search.h header since it > is only used in one file (hashtable.c). Remove it from the header and > change it to a struct. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > include/search.h | 2 +- > lib/hashtable.c | 7 ++++--- > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/include/search.h b/include/search.h > index efa8bcbef6..c99648f80b 100644 > --- a/include/search.h > +++ b/include/search.h > @@ -42,7 +42,7 @@ struct env_entry { > > /* Data type for reentrant functions. */ > struct hsearch_data { > - struct _ENTRY *table; > + struct env_entry_node *table; Don't you need an opaque definition of this? Also, there is an opaque definition of _ENTRY in this file that needs to go away. > unsigned int size; > unsigned int filled; > /* > diff --git a/lib/hashtable.c b/lib/hashtable.c > index c77b68f4e6..1093d8adaa 100644 > --- a/lib/hashtable.c > +++ b/lib/hashtable.c > @@ -59,10 +59,10 @@ > * which describes the current status. > */ > > -typedef struct _ENTRY { > +struct env_entry_node { > int used; > struct env_entry entry; > -} _ENTRY; > +}; > > > static void _hdelete(const char *key, struct hsearch_data *htab, > @@ -120,7 +120,8 @@ int hcreate_r(size_t nel, struct hsearch_data *htab) > htab->filled = 0; > > /* allocate memory and zero out */ > - htab->table = (_ENTRY *) calloc(htab->size + 1, sizeof(_ENTRY)); > + htab->table = (struct env_entry_node *)calloc(htab->size + 1, > + sizeof(struct env_entry_node)); > if (htab->table == NULL) > return 0; > > -- > 2.22.0.709.g102302147b-goog > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
Hi Joe, On Tue, 30 Jul 2019 at 15:35, Joe Hershberger <joe.hershberger@ni.com> wrote: > > On Sun, Jul 28, 2019 at 9:28 AM Simon Glass <sjg@chromium.org> wrote: > > > > This typedef does not need to be defined in the search.h header since it > > is only used in one file (hashtable.c). Remove it from the header and > > change it to a struct. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > include/search.h | 2 +- > > lib/hashtable.c | 7 ++++--- > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/include/search.h b/include/search.h > > index efa8bcbef6..c99648f80b 100644 > > --- a/include/search.h > > +++ b/include/search.h > > @@ -42,7 +42,7 @@ struct env_entry { > > > > /* Data type for reentrant functions. */ > > struct hsearch_data { > > - struct _ENTRY *table; > > + struct env_entry_node *table; > > Don't you need an opaque definition of this? I don't see why. We can just use struct env_entry_node which is opaque if the definition is not available. > > Also, there is an opaque definition of _ENTRY in this file that needs > to go away. Where is that? I can't see it. > > > unsigned int size; > > unsigned int filled; > > /* > > diff --git a/lib/hashtable.c b/lib/hashtable.c > > index c77b68f4e6..1093d8adaa 100644 > > --- a/lib/hashtable.c > > +++ b/lib/hashtable.c > > @@ -59,10 +59,10 @@ > > * which describes the current status. > > */ > > > > -typedef struct _ENTRY { > > +struct env_entry_node { > > int used; > > struct env_entry entry; > > -} _ENTRY; > > +}; > > > > > > static void _hdelete(const char *key, struct hsearch_data *htab, > > @@ -120,7 +120,8 @@ int hcreate_r(size_t nel, struct hsearch_data *htab) > > htab->filled = 0; > > > > /* allocate memory and zero out */ > > - htab->table = (_ENTRY *) calloc(htab->size + 1, sizeof(_ENTRY)); > > + htab->table = (struct env_entry_node *)calloc(htab->size + 1, > > + sizeof(struct env_entry_node)); > > if (htab->table == NULL) > > return 0; > > > > -- > > 2.22.0.709.g102302147b-goog > > Regards, Simon
On Wed, Jul 31, 2019 at 3:57 PM Simon Glass <sjg@chromium.org> wrote: > > Hi Joe, > > On Tue, 30 Jul 2019 at 15:35, Joe Hershberger <joe.hershberger@ni.com> wrote: > > > > On Sun, Jul 28, 2019 at 9:28 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > This typedef does not need to be defined in the search.h header since it > > > is only used in one file (hashtable.c). Remove it from the header and > > > change it to a struct. > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > --- > > > > > > include/search.h | 2 +- > > > lib/hashtable.c | 7 ++++--- > > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > > > diff --git a/include/search.h b/include/search.h > > > index efa8bcbef6..c99648f80b 100644 > > > --- a/include/search.h > > > +++ b/include/search.h > > > @@ -42,7 +42,7 @@ struct env_entry { > > > > > > /* Data type for reentrant functions. */ > > > struct hsearch_data { > > > - struct _ENTRY *table; > > > + struct env_entry_node *table; > > > > Don't you need an opaque definition of this? > > I don't see why. We can just use struct env_entry_node which is opaque > if the definition is not available. I agree, but doesn't it need to be defined? Maybe the misunderstanding is happening because of the intermediate state of things through out this series. > > > > > Also, there is an opaque definition of _ENTRY in this file that needs > > to go away. > > Where is that? I can't see it. I'm looking at master... "include/search.h" line 42 of 123 > > > > > > unsigned int size; > > > unsigned int filled; > > > /* > > > diff --git a/lib/hashtable.c b/lib/hashtable.c > > > index c77b68f4e6..1093d8adaa 100644 > > > --- a/lib/hashtable.c > > > +++ b/lib/hashtable.c > > > @@ -59,10 +59,10 @@ > > > * which describes the current status. > > > */ > > > > > > -typedef struct _ENTRY { > > > +struct env_entry_node { > > > int used; > > > struct env_entry entry; > > > -} _ENTRY; > > > +}; > > > > > > > > > static void _hdelete(const char *key, struct hsearch_data *htab, > > > @@ -120,7 +120,8 @@ int hcreate_r(size_t nel, struct hsearch_data *htab) > > > htab->filled = 0; > > > > > > /* allocate memory and zero out */ > > > - htab->table = (_ENTRY *) calloc(htab->size + 1, sizeof(_ENTRY)); > > > + htab->table = (struct env_entry_node *)calloc(htab->size + 1, > > > + sizeof(struct env_entry_node)); > > > if (htab->table == NULL) > > > return 0; > > > > > > -- > > > 2.22.0.709.g102302147b-goog > > > > > Regards, > Simon > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
Hi, On Wed, 31 Jul 2019 at 15:07, Joe Hershberger <joe.hershberger@ni.com> wrote: > > On Wed, Jul 31, 2019 at 3:57 PM Simon Glass <sjg@chromium.org> wrote: > > > > Hi Joe, > > > > On Tue, 30 Jul 2019 at 15:35, Joe Hershberger <joe.hershberger@ni.com> wrote: > > > > > > On Sun, Jul 28, 2019 at 9:28 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > This typedef does not need to be defined in the search.h header since it > > > > is only used in one file (hashtable.c). Remove it from the header and > > > > change it to a struct. > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > --- > > > > > > > > include/search.h | 2 +- > > > > lib/hashtable.c | 7 ++++--- > > > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/include/search.h b/include/search.h > > > > index efa8bcbef6..c99648f80b 100644 > > > > --- a/include/search.h > > > > +++ b/include/search.h > > > > @@ -42,7 +42,7 @@ struct env_entry { > > > > > > > > /* Data type for reentrant functions. */ > > > > struct hsearch_data { > > > > - struct _ENTRY *table; > > > > + struct env_entry_node *table; > > > > > > Don't you need an opaque definition of this? > > > > I don't see why. We can just use struct env_entry_node which is opaque > > if the definition is not available. > > I agree, but doesn't it need to be defined? Maybe the misunderstanding > is happening because of the intermediate state of things through out > this series. (yes, I had trouble figuring out how to split this series up so people could actually review it!) I don't think it needs to be defined separate here, since it is not in a function scope, so mentioning it inside a struct seems to work OK. > > > > > > > > > Also, there is an opaque definition of _ENTRY in this file that needs > > > to go away. > > > > Where is that? I can't see it. > > I'm looking at master... "include/search.h" line 42 of 123 OK, I see. That is removed in the previous patch "env: Drop the ENTRY typdef" Regards, Simon
On Wed, Jul 31, 2019 at 4:56 PM Simon Glass <sjg@chromium.org> wrote: > > Hi, > > On Wed, 31 Jul 2019 at 15:07, Joe Hershberger <joe.hershberger@ni.com> wrote: > > > > On Wed, Jul 31, 2019 at 3:57 PM Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Joe, > > > > > > On Tue, 30 Jul 2019 at 15:35, Joe Hershberger <joe.hershberger@ni.com> wrote: > > > > > > > > On Sun, Jul 28, 2019 at 9:28 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > This typedef does not need to be defined in the search.h header since it > > > > > is only used in one file (hashtable.c). Remove it from the header and > > > > > change it to a struct. > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > > --- > > > > > > > > > > include/search.h | 2 +- > > > > > lib/hashtable.c | 7 ++++--- > > > > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/include/search.h b/include/search.h > > > > > index efa8bcbef6..c99648f80b 100644 > > > > > --- a/include/search.h > > > > > +++ b/include/search.h > > > > > @@ -42,7 +42,7 @@ struct env_entry { > > > > > > > > > > /* Data type for reentrant functions. */ > > > > > struct hsearch_data { > > > > > - struct _ENTRY *table; > > > > > + struct env_entry_node *table; > > > > > > > > Don't you need an opaque definition of this? > > > > > > I don't see why. We can just use struct env_entry_node which is opaque > > > if the definition is not available. > > > > I agree, but doesn't it need to be defined? Maybe the misunderstanding > > is happening because of the intermediate state of things through out > > this series. > > (yes, I had trouble figuring out how to split this series up so people > could actually review it!) Well, I think you did a fantastic job at it. > > I don't think it needs to be defined separate here, since it is not in > a function scope, so mentioning it inside a struct seems to work OK. Sounds good to me, if it works! > > > > > > > > > > > > > Also, there is an opaque definition of _ENTRY in this file that needs > > > > to go away. > > > > > > Where is that? I can't see it. > > > > I'm looking at master... "include/search.h" line 42 of 123 > > OK, I see. That is removed in the previous patch "env: Drop the ENTRY typdef" Ah, ok... It would be better to group it with this patch since _ENTRY shouldn't be related to ENTRY. > > Regards, > Simon > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
Hi Joe, On Wed, 31 Jul 2019 at 16:03, Joe Hershberger <joe.hershberger@ni.com> wrote: > > On Wed, Jul 31, 2019 at 4:56 PM Simon Glass <sjg@chromium.org> wrote: > > > > Hi, > > > > On Wed, 31 Jul 2019 at 15:07, Joe Hershberger <joe.hershberger@ni.com> wrote: > > > > > > On Wed, Jul 31, 2019 at 3:57 PM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Joe, > > > > > > > > On Tue, 30 Jul 2019 at 15:35, Joe Hershberger <joe.hershberger@ni.com> wrote: > > > > > > > > > > On Sun, Jul 28, 2019 at 9:28 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > This typedef does not need to be defined in the search.h header since it > > > > > > is only used in one file (hashtable.c). Remove it from the header and > > > > > > change it to a struct. > > > > > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > > > --- > > > > > > > > > > > > include/search.h | 2 +- > > > > > > lib/hashtable.c | 7 ++++--- > > > > > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/include/search.h b/include/search.h > > > > > > index efa8bcbef6..c99648f80b 100644 > > > > > > --- a/include/search.h > > > > > > +++ b/include/search.h > > > > > > @@ -42,7 +42,7 @@ struct env_entry { > > > > > > > > > > > > /* Data type for reentrant functions. */ > > > > > > struct hsearch_data { > > > > > > - struct _ENTRY *table; > > > > > > + struct env_entry_node *table; > > > > > > > > > > Don't you need an opaque definition of this? > > > > > > > > I don't see why. We can just use struct env_entry_node which is opaque > > > > if the definition is not available. > > > > > > I agree, but doesn't it need to be defined? Maybe the misunderstanding > > > is happening because of the intermediate state of things through out > > > this series. > > > > (yes, I had trouble figuring out how to split this series up so people > > could actually review it!) > > Well, I think you did a fantastic job at it. > > > > > I don't think it needs to be defined separate here, since it is not in > > a function scope, so mentioning it inside a struct seems to work OK. > > Sounds good to me, if it works! > > > > > > > > > > > > > > > > > > Also, there is an opaque definition of _ENTRY in this file that needs > > > > > to go away. > > > > > > > > Where is that? I can't see it. > > > > > > I'm looking at master... "include/search.h" line 42 of 123 > > > > OK, I see. That is removed in the previous patch "env: Drop the ENTRY typdef" > > Ah, ok... It would be better to group it with this patch since _ENTRY > shouldn't be related to ENTRY. Ah yes of course. Fixed. I'll just send a few updated v3 patches. Regards, Simon
diff --git a/include/search.h b/include/search.h index efa8bcbef6..c99648f80b 100644 --- a/include/search.h +++ b/include/search.h @@ -42,7 +42,7 @@ struct env_entry { /* Data type for reentrant functions. */ struct hsearch_data { - struct _ENTRY *table; + struct env_entry_node *table; unsigned int size; unsigned int filled; /* diff --git a/lib/hashtable.c b/lib/hashtable.c index c77b68f4e6..1093d8adaa 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -59,10 +59,10 @@ * which describes the current status. */ -typedef struct _ENTRY { +struct env_entry_node { int used; struct env_entry entry; -} _ENTRY; +}; static void _hdelete(const char *key, struct hsearch_data *htab, @@ -120,7 +120,8 @@ int hcreate_r(size_t nel, struct hsearch_data *htab) htab->filled = 0; /* allocate memory and zero out */ - htab->table = (_ENTRY *) calloc(htab->size + 1, sizeof(_ENTRY)); + htab->table = (struct env_entry_node *)calloc(htab->size + 1, + sizeof(struct env_entry_node)); if (htab->table == NULL) return 0;
This typedef does not need to be defined in the search.h header since it is only used in one file (hashtable.c). Remove it from the header and change it to a struct. Signed-off-by: Simon Glass <sjg@chromium.org> --- include/search.h | 2 +- lib/hashtable.c | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-)