Re: [PATCH v2 04/13] env: Change env_match() to static and remove from header

Simon Glass
2021-10-15T00:41:19Z

On Thu, 14 Oct 2021 at 12:24, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Marek,
>
> On Thu, 14 Oct 2021 at 10:06, Marek Behún <kabel@kernel.org> wrote:
> >
> > On Thu, 14 Oct 2021 09:11:08 -0600
> > Simon Glass <sjg@chromium.org> wrote:
> >
> > > Hi Marek,
> > >
> > > On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
> > > >
> > > > From: Marek Behún <marek.behun@nic.cz>
> > > >
> > > > This function was used by other parts of U-Boot in the past when
> > > > environment was read from underlying device one character at a time.
> > > >
> > > > This is not the case anymore.
> > > >
> > > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > > ---
> > > > cmd/nvedit.c | 30 +++++++++++++++---------------
> > > > include/env.h | 11 -----------
> > > > 2 files changed, 15 insertions(+), 26 deletions(-)
> > > >
> > > > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > > > index ddc715b4f9..742e0924af 100644
> > > > --- a/cmd/nvedit.c
> > > > +++ b/cmd/nvedit.c
> > > > @@ -706,6 +706,21 @@ char *from_env(const char *envvar)
> > > > return ret;
> > > > }
> > > >
> > >
> > > Please can you add the function comment here? We don't want to lose
> > > it.
> >
> > Simon, the comment is invalid (the function does something
> > different from what the comment says) and the function is only used as a
> > helper by env_get_f(), which comes right after it. The function is
> > refactored and renamend in subsequent patches, and its purpose seems
> > obvious to me.
> >
> > Should I really leave the comment there?
>
> That's fine but it would be good to mention that in the commit message
> explicitly.
>
> Also you add matching_name_get_value(). Can you add a comment to that, then?
>
Reviewed-by: Simon Glass <sjg@chromium.org>