Jaben, If and Else are using the same API. I would like to remove CommandInit() completely from the library and update the callers for code cleanup. Do you know whether there will be any potential issue for this update?
Thanks/Ray > -----Original Message----- > From: Carsey, Jaben > Sent: Thursday, July 7, 2016 10:42 PM > To: Ni, Ruiyu <ruiyu...@intel.com> > Cc: edk2-devel-01 <edk2-de...@ml01.01.org>; Laszlo Ersek > <ler...@redhat.com>; Qiu, Shumin <shumin....@intel.com>; Carsey, Jaben > <jaben.car...@intel.com> > Subject: RE: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side > effects in ASSERT_EFI_ERROR() > > We should see why If and Else are using this different API for sure. I don't > remember why. > > I would be happy to remove it completely if there is no objection. > > -Jaben > > > -----Original Message----- > > From: Ni, Ruiyu > > Sent: Thursday, July 07, 2016 1:20 AM > > To: Carsey, Jaben <jaben.car...@intel.com> > > Cc: Carsey, Jaben <jaben.car...@intel.com>; edk2-devel-01 <edk2- > > de...@ml01.01.org>; Laszlo Ersek <ler...@redhat.com>; Qiu, Shumin > > <shumin....@intel.com> > > Subject: RE: [edk2] [PATCH 4/6] ShellPkg: don't call functions with > > side effects in ASSERT_EFI_ERROR() > > Importance: High > > > > Jaben, > > CommandInit() API initializes gUnicodeCollation pointer. > > But ShellCommandLibConstructor() also initializes the > > gUnicodeCollation pointer. > > > > Can we just remove CommandInit() from this library? And update all callers? > > > > Thanks/Ray > > > > > -----Original Message----- > > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf > > > Of Carsey, Jaben > > > Sent: Thursday, June 30, 2016 11:13 PM > > > To: Laszlo Ersek <ler...@redhat.com>; Qiu, Shumin > > <shumin....@intel.com> > > > Cc: Carsey, Jaben <jaben.car...@intel.com>; edk2-devel-01 <edk2- > > > de...@ml01.01.org> > > > Subject: Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with > > > side effects in ASSERT_EFI_ERROR() > > > > > > Reviewed-by: Jaben Carsey <jaben.car...@intel.com> > > > > > > > -----Original Message----- > > > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > > > Sent: Thursday, June 30, 2016 4:07 AM > > > > To: Carsey, Jaben <jaben.car...@intel.com>; Qiu, Shumin > > > > <shumin....@intel.com> > > > > Cc: edk2-devel-01 <edk2-de...@ml01.01.org> > > > > Subject: Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions > > > > with side effects in ASSERT_EFI_ERROR() > > > > Importance: High > > > > > > > > Jaben, Shumin, > > > > > > > > On 06/28/16 15:25, Laszlo Ersek wrote: > > > > > When ASSERT_EFI_ERROR() is compiled out, dependent on build > > > > > flags, only the status checking should be removed; the function > > > > > calls should > > > stay. > > > > > > > > > > Cc: Jaben Carsey <jaben.car...@intel.com> > > > > > Cc: Shumin Qiu <shumin....@intel.com> > > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > > > > Signed-off-by: Laszlo Ersek <ler...@redhat.com> > > > > > --- > > > > > > > > > > Notes: > > > > > build tested > > > > > > > > > > ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 ++++++++-- > > > > > ShellPkg/Library/UefiShellLib/UefiShellLib.c | 5 ++++- > > > > > 2 files changed, 12 insertions(+), 3 deletions(-) > > > > > > > > Can I please get a maintainer review for this patch? > > > > > > > > Thanks > > > > Laszlo > > > > > > > > > diff --git a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c > > > > b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c > > > > > index 7abfd8944b92..dc96bffde7d3 100644 > > > > > --- a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c > > > > > +++ b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c > > > > > @@ -991,8 +991,11 @@ ShellCommandRunElse ( > > > > > IN EFI_SYSTEM_TABLE *SystemTable > > > > > ) > > > > > { > > > > > + EFI_STATUS Status; > > > > > SCRIPT_FILE *CurrentScriptFile; > > > > > - ASSERT_EFI_ERROR(CommandInit()); > > > > > + > > > > > + Status = CommandInit (); > > > > > + ASSERT_EFI_ERROR (Status); > > > > > > > > > > if (gEfiShellParametersProtocol->Argc > 1) { > > > > > ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN > > > > > (STR_GEN_TOO_MANY), > > > > gShellLevel1HiiHandle, L"if"); > > > > > @@ -1066,8 +1069,11 @@ ShellCommandRunEndIf ( > > > > > IN EFI_SYSTEM_TABLE *SystemTable > > > > > ) > > > > > { > > > > > + EFI_STATUS Status; > > > > > SCRIPT_FILE *CurrentScriptFile; > > > > > - ASSERT_EFI_ERROR(CommandInit()); > > > > > + > > > > > + Status = CommandInit (); > > > > > + ASSERT_EFI_ERROR (Status); > > > > > > > > > > if (gEfiShellParametersProtocol->Argc > 1) { > > > > > ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN > > > > > (STR_GEN_TOO_MANY), > > > > gShellLevel1HiiHandle, L"if"); > > > > > diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c > > > > b/ShellPkg/Library/UefiShellLib/UefiShellLib.c > > > > > index cf89a4ac87ed..35a1a7169c8b 100644 > > > > > --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c > > > > > +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c > > > > > @@ -373,6 +373,8 @@ EFIAPI > > > > > ShellInitialize ( > > > > > ) > > > > > { > > > > > + EFI_STATUS Status; > > > > > + > > > > > // > > > > > // if auto initialize is not false then skip > > > > > // > > > > > @@ -383,7 +385,8 @@ ShellInitialize ( > > > > > // > > > > > // deinit the current stuff > > > > > // > > > > > - ASSERT_EFI_ERROR(ShellLibDestructor(gImageHandle, gST)); > > > > > + Status = ShellLibDestructor (gImageHandle, gST); > > > > > + ASSERT_EFI_ERROR (Status); > > > > > > > > > > // > > > > > // init the new stuff > > > > > > > > > > > _______________________________________________ > > > edk2-devel mailing list > > > edk2-devel@lists.01.org > > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
