[patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Dandan Bi]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1306

In function UiDisplayMenu, the NewPos ptr which used to point to the
highlight menu entry. It will always point to the menu entry which
need to be highlighted or the gMenuOption menu if the highlight menu
is not found.
So we can remove the NULL ptr check for NewPos in this function.
And add the ASSERT code to avoid if any false positive reports
of NULL pointer dereference issue raised from static analysis.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
---
 MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
index 7390f954b6..44f087fe01 100644
--- a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
+++ b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
@@ -2880,10 +2880,11 @@ UiDisplayMenu (
       //             MenuOption is set to NULL in Repaint
       // NewPos:     Current menu option that need to hilight
       //
       ControlFlag = CfUpdateHelpString;
 
+      ASSERT (NewPos != NULL);
       UpdateHighlightMenuInfo(NewPos, TopOfScreen, SkipValue);
 
       if (SkipHighLight) {
         SkipHighLight = FALSE;
         MenuOption    = SavedMenuOption;
@@ -2908,11 +2909,11 @@ UiDisplayMenu (
         Temp2 = SkipValue;
       } else {
         Temp2 = 0;
       }
 
-      if (NewPos != NULL && (MenuOption == NULL || NewPos != &MenuOption->Link)) {
+      if (MenuOption == NULL || NewPos != &MenuOption->Link) {
         if (MenuOption != NULL) {
           //
           // Remove the old highlight menu.
           //
           Status = DisplayOneMenu (MenuOption,
-- 
2.18.0.windows.1

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com>

> -----Original Message-----
> From: Bi, Dandan
> Sent: Wednesday, November 7, 2018 10:53 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>
> Subject: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1306
> 
> In function UiDisplayMenu, the NewPos ptr which used to point to the
> highlight menu entry. It will always point to the menu entry which
> need to be highlighted or the gMenuOption menu if the highlight menu
> is not found.
> So we can remove the NULL ptr check for NewPos in this function.
> And add the ASSERT code to avoid if any false positive reports
> of NULL pointer dereference issue raised from static analysis.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> ---
>  MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> index 7390f954b6..44f087fe01 100644
> --- a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> +++ b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> @@ -2880,10 +2880,11 @@ UiDisplayMenu (
>        //             MenuOption is set to NULL in Repaint
>        // NewPos:     Current menu option that need to hilight
>        //
>        ControlFlag = CfUpdateHelpString;
> 
> +      ASSERT (NewPos != NULL);
>        UpdateHighlightMenuInfo(NewPos, TopOfScreen, SkipValue);
> 
>        if (SkipHighLight) {
>          SkipHighLight = FALSE;
>          MenuOption    = SavedMenuOption;
> @@ -2908,11 +2909,11 @@ UiDisplayMenu (
>          Temp2 = SkipValue;
>        } else {
>          Temp2 = 0;
>        }
> 
> -      if (NewPos != NULL && (MenuOption == NULL || NewPos != &MenuOption->Link)) {
> +      if (MenuOption == NULL || NewPos != &MenuOption->Link) {
>          if (MenuOption != NULL) {
>            //
>            // Remove the old highlight menu.
>            //
>            Status = DisplayOneMenu (MenuOption,
> --
> 2.18.0.windows.1

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: Bi, Dandan 
Sent: Wednesday, November 7, 2018 10:53 PM
To: edk2-devel@lists.01.org
Cc: Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1306

In function UiDisplayMenu, the NewPos ptr which used to point to the highlight menu entry. It will always point to the menu entry which need to be highlighted or the gMenuOption menu if the highlight menu is not found.
So we can remove the NULL ptr check for NewPos in this function.
And add the ASSERT code to avoid if any false positive reports of NULL pointer dereference issue raised from static analysis.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dandan Bi <dandan.bi@intel.com>
---
 MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
index 7390f954b6..44f087fe01 100644
--- a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
+++ b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
@@ -2880,10 +2880,11 @@ UiDisplayMenu (
       //             MenuOption is set to NULL in Repaint
       // NewPos:     Current menu option that need to hilight
       //
       ControlFlag = CfUpdateHelpString;
 
+      ASSERT (NewPos != NULL);
       UpdateHighlightMenuInfo(NewPos, TopOfScreen, SkipValue);
 
       if (SkipHighLight) {
         SkipHighLight = FALSE;
         MenuOption    = SavedMenuOption;
@@ -2908,11 +2909,11 @@ UiDisplayMenu (
         Temp2 = SkipValue;
       } else {
         Temp2 = 0;
       }
 
-      if (NewPos != NULL && (MenuOption == NULL || NewPos != &MenuOption->Link)) {
+      if (MenuOption == NULL || NewPos != &MenuOption->Link) {
         if (MenuOption != NULL) {
           //
           // Remove the old highlight menu.
           //
           Status = DisplayOneMenu (MenuOption,
--
2.18.0.windows.1

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Ard Biesheuvel]

On 8 November 2018 at 02:09, Zeng, Star <star.zeng@intel.com> wrote:
> Reviewed-by: Star Zeng <star.zeng@intel.com>
>
> -----Original Message-----
> From: Bi, Dandan
> Sent: Wednesday, November 7, 2018 10:53 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1306
>
> In function UiDisplayMenu, the NewPos ptr which used to point to the highlight menu entry. It will always point to the menu entry which need to be highlighted or the gMenuOption menu if the highlight menu is not found.
> So we can remove the NULL ptr check for NewPos in this function.
> And add the ASSERT code to avoid if any false positive reports of NULL pointer dereference issue raised from static analysis.
>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>

Why was this patch merged today? Surely, this doesn't meet the hard
freeze requirements ?

> ---
>  MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> index 7390f954b6..44f087fe01 100644
> --- a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> +++ b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> @@ -2880,10 +2880,11 @@ UiDisplayMenu (
>        //             MenuOption is set to NULL in Repaint
>        // NewPos:     Current menu option that need to hilight
>        //
>        ControlFlag = CfUpdateHelpString;
>
> +      ASSERT (NewPos != NULL);
>        UpdateHighlightMenuInfo(NewPos, TopOfScreen, SkipValue);
>
>        if (SkipHighLight) {
>          SkipHighLight = FALSE;
>          MenuOption    = SavedMenuOption;
> @@ -2908,11 +2909,11 @@ UiDisplayMenu (
>          Temp2 = SkipValue;
>        } else {
>          Temp2 = 0;
>        }
>
> -      if (NewPos != NULL && (MenuOption == NULL || NewPos != &MenuOption->Link)) {
> +      if (MenuOption == NULL || NewPos != &MenuOption->Link) {
>          if (MenuOption != NULL) {
>            //
>            // Remove the old highlight menu.
>            //
>            Status = DisplayOneMenu (MenuOption,
> --
> 2.18.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Gao, Liming]

Ard:
  This is a small fix. And, this patch is sent before the hard freeze. It is the low risk for this release. So, I push it. 

Thanks
Liming
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Friday, November 09, 2018 2:25 AM
>To: Zeng, Star <star.zeng@intel.com>
>Cc: Bi, Dandan <dandan.bi@intel.com>; edk2-devel@lists.01.org; Wu, Hao A
><hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming
><liming.gao@intel.com>
>Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless
>NULL ptr check for NewPos
>
>On 8 November 2018 at 02:09, Zeng, Star <star.zeng@intel.com> wrote:
>> Reviewed-by: Star Zeng <star.zeng@intel.com>
>>
>> -----Original Message-----
>> From: Bi, Dandan
>> Sent: Wednesday, November 7, 2018 10:53 PM
>> To: edk2-devel@lists.01.org
>> Cc: Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>;
>Zeng, Star <star.zeng@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
>> Subject: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr
>check for NewPos
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1306
>>
>> In function UiDisplayMenu, the NewPos ptr which used to point to the
>highlight menu entry. It will always point to the menu entry which need to be
>highlighted or the gMenuOption menu if the highlight menu is not found.
>> So we can remove the NULL ptr check for NewPos in this function.
>> And add the ASSERT code to avoid if any false positive reports of NULL
>pointer dereference issue raised from static analysis.
>>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Cc: Hao Wu <hao.a.wu@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
>
>Why was this patch merged today? Surely, this doesn't meet the hard
>freeze requirements ?
>
>> ---
>>  MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
>b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
>> index 7390f954b6..44f087fe01 100644
>> --- a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
>> +++ b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
>> @@ -2880,10 +2880,11 @@ UiDisplayMenu (
>>        //             MenuOption is set to NULL in Repaint
>>        // NewPos:     Current menu option that need to hilight
>>        //
>>        ControlFlag = CfUpdateHelpString;
>>
>> +      ASSERT (NewPos != NULL);
>>        UpdateHighlightMenuInfo(NewPos, TopOfScreen, SkipValue);
>>
>>        if (SkipHighLight) {
>>          SkipHighLight = FALSE;
>>          MenuOption    = SavedMenuOption;
>> @@ -2908,11 +2909,11 @@ UiDisplayMenu (
>>          Temp2 = SkipValue;
>>        } else {
>>          Temp2 = 0;
>>        }
>>
>> -      if (NewPos != NULL && (MenuOption == NULL || NewPos !=
>&MenuOption->Link)) {
>> +      if (MenuOption == NULL || NewPos != &MenuOption->Link) {
>>          if (MenuOption != NULL) {
>>            //
>>            // Remove the old highlight menu.
>>            //
>>            Status = DisplayOneMenu (MenuOption,
>> --
>> 2.18.0.windows.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Ard Biesheuvel]

On 9 November 2018 at 01:19, Gao, Liming <liming.gao@intel.com> wrote:
> Ard:
>   This is a small fix. And, this patch is sent before the hard freeze. It is the low risk for this release. So, I push it.
>

OK, fair enough.

>>-----Original Message-----
>>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>Sent: Friday, November 09, 2018 2:25 AM
>>To: Zeng, Star <star.zeng@intel.com>
>>Cc: Bi, Dandan <dandan.bi@intel.com>; edk2-devel@lists.01.org; Wu, Hao A
>><hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming
>><liming.gao@intel.com>
>>Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless
>>NULL ptr check for NewPos
>>
>>On 8 November 2018 at 02:09, Zeng, Star <star.zeng@intel.com> wrote:
>>> Reviewed-by: Star Zeng <star.zeng@intel.com>
>>>
>>> -----Original Message-----
>>> From: Bi, Dandan
>>> Sent: Wednesday, November 7, 2018 10:53 PM
>>> To: edk2-devel@lists.01.org
>>> Cc: Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>;
>>Zeng, Star <star.zeng@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
>>> Subject: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr
>>check for NewPos
>>>
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1306
>>>
>>> In function UiDisplayMenu, the NewPos ptr which used to point to the
>>highlight menu entry. It will always point to the menu entry which need to be
>>highlighted or the gMenuOption menu if the highlight menu is not found.
>>> So we can remove the NULL ptr check for NewPos in this function.
>>> And add the ASSERT code to avoid if any false positive reports of NULL
>>pointer dereference issue raised from static analysis.
>>>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>> Cc: Hao Wu <hao.a.wu@intel.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
>>
>>Why was this patch merged today? Surely, this doesn't meet the hard
>>freeze requirements ?
>>
>>> ---
>>>  MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
>>b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
>>> index 7390f954b6..44f087fe01 100644
>>> --- a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
>>> +++ b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
>>> @@ -2880,10 +2880,11 @@ UiDisplayMenu (
>>>        //             MenuOption is set to NULL in Repaint
>>>        // NewPos:     Current menu option that need to hilight
>>>        //
>>>        ControlFlag = CfUpdateHelpString;
>>>
>>> +      ASSERT (NewPos != NULL);
>>>        UpdateHighlightMenuInfo(NewPos, TopOfScreen, SkipValue);
>>>
>>>        if (SkipHighLight) {
>>>          SkipHighLight = FALSE;
>>>          MenuOption    = SavedMenuOption;
>>> @@ -2908,11 +2909,11 @@ UiDisplayMenu (
>>>          Temp2 = SkipValue;
>>>        } else {
>>>          Temp2 = 0;
>>>        }
>>>
>>> -      if (NewPos != NULL && (MenuOption == NULL || NewPos !=
>>&MenuOption->Link)) {
>>> +      if (MenuOption == NULL || NewPos != &MenuOption->Link) {
>>>          if (MenuOption != NULL) {
>>>            //
>>>            // Remove the old highlight menu.
>>>            //
>>>            Status = DisplayOneMenu (MenuOption,
>>> --
>>> 2.18.0.windows.1
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Leif Lindholm]

On Fri, Nov 09, 2018 at 07:56:07AM +0100, Ard Biesheuvel wrote:
> On 9 November 2018 at 01:19, Gao, Liming <liming.gao@intel.com> wrote:
> > Ard:
> >   This is a small fix. And, this patch is sent before the hard
> >   freeze. It is the low risk for this release. So, I push it.
> 
> OK, fair enough.

I don't agree actually.

https://github.com/tianocore/tianocore.github.io/wiki/HardFeatureFreeze
specifies clearly that only bug fixes are permitted in during hard
freeze. Maybe we could document that a bit more explicitly, but this
patch was no bugfix. It should not have gone in.

By my interpretation, it would not even fulfill the requirements for
https://github.com/lersek/edk2/wiki/SoftFeatureFreeze:
"By the date of the soft feature freeze, developers must have sent
their patches to the mailing list and received positive maintainer
reviews."
Soft feature freeze was 1 November. The patch was sent out 7 November.
It received reviews 8 November (after the start of the hard freeze).

The point of these freezes is that sometimes patches are wrong. And
sometimes patches that look correct, are not correct. If we start
making exceptions because "oh, it's trivial", that means we get these
patches into the tree with much reduced time for anyone to catch any
adverse effects before we make the stable tag. And at that point, the
stable tag no longer has value.

(I am much more flexible on the topic of updating documentation, like
Maintainers.txt, but even there we must be very careful.)

Regards,

Leif

> >>-----Original Message-----
> >>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >>Sent: Friday, November 09, 2018 2:25 AM
> >>To: Zeng, Star <star.zeng@intel.com>
> >>Cc: Bi, Dandan <dandan.bi@intel.com>; edk2-devel@lists.01.org; Wu, Hao A
> >><hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming
> >><liming.gao@intel.com>
> >>Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless
> >>NULL ptr check for NewPos
> >>
> >>On 8 November 2018 at 02:09, Zeng, Star <star.zeng@intel.com> wrote:
> >>> Reviewed-by: Star Zeng <star.zeng@intel.com>
> >>>
> >>> -----Original Message-----
> >>> From: Bi, Dandan
> >>> Sent: Wednesday, November 7, 2018 10:53 PM
> >>> To: edk2-devel@lists.01.org
> >>> Cc: Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> >>Zeng, Star <star.zeng@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> >>> Subject: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr
> >>check for NewPos
> >>>
> >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1306
> >>>
> >>> In function UiDisplayMenu, the NewPos ptr which used to point to the
> >>highlight menu entry. It will always point to the menu entry which need to be
> >>highlighted or the gMenuOption menu if the highlight menu is not found.
> >>> So we can remove the NULL ptr check for NewPos in this function.
> >>> And add the ASSERT code to avoid if any false positive reports of NULL
> >>pointer dereference issue raised from static analysis.
> >>>
> >>> Cc: Liming Gao <liming.gao@intel.com>
> >>> Cc: Eric Dong <eric.dong@intel.com>
> >>> Cc: Star Zeng <star.zeng@intel.com>
> >>> Cc: Hao Wu <hao.a.wu@intel.com>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> >>
> >>Why was this patch merged today? Surely, this doesn't meet the hard
> >>freeze requirements ?
> >>
> >>> ---
> >>>  MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> >>b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> >>> index 7390f954b6..44f087fe01 100644
> >>> --- a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> >>> +++ b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> >>> @@ -2880,10 +2880,11 @@ UiDisplayMenu (
> >>>        //             MenuOption is set to NULL in Repaint
> >>>        // NewPos:     Current menu option that need to hilight
> >>>        //
> >>>        ControlFlag = CfUpdateHelpString;
> >>>
> >>> +      ASSERT (NewPos != NULL);
> >>>        UpdateHighlightMenuInfo(NewPos, TopOfScreen, SkipValue);
> >>>
> >>>        if (SkipHighLight) {
> >>>          SkipHighLight = FALSE;
> >>>          MenuOption    = SavedMenuOption;
> >>> @@ -2908,11 +2909,11 @@ UiDisplayMenu (
> >>>          Temp2 = SkipValue;
> >>>        } else {
> >>>          Temp2 = 0;
> >>>        }
> >>>
> >>> -      if (NewPos != NULL && (MenuOption == NULL || NewPos !=
> >>&MenuOption->Link)) {
> >>> +      if (MenuOption == NULL || NewPos != &MenuOption->Link) {
> >>>          if (MenuOption != NULL) {
> >>>            //
> >>>            // Remove the old highlight menu.
> >>>            //
> >>>            Status = DisplayOneMenu (MenuOption,
> >>> --
> >>> 2.18.0.windows.1
> >>>
> >>> _______________________________________________
> >>> 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

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Laszlo Ersek]

On 11/09/18 11:49, Leif Lindholm wrote:
> On Fri, Nov 09, 2018 at 07:56:07AM +0100, Ard Biesheuvel wrote:
>> On 9 November 2018 at 01:19, Gao, Liming <liming.gao@intel.com> wrote:
>>> Ard:
>>>   This is a small fix. And, this patch is sent before the hard
>>>   freeze. It is the low risk for this release. So, I push it.
>>
>> OK, fair enough.
> 
> I don't agree actually.
> 
> https://github.com/tianocore/tianocore.github.io/wiki/HardFeatureFreeze
> specifies clearly that only bug fixes are permitted in during hard
> freeze. Maybe we could document that a bit more explicitly, but this
> patch was no bugfix. It should not have gone in.
> 
> By my interpretation, it would not even fulfill the requirements for
> https://github.com/lersek/edk2/wiki/SoftFeatureFreeze:
> "By the date of the soft feature freeze, developers must have sent
> their patches to the mailing list and received positive maintainer
> reviews."
> Soft feature freeze was 1 November. The patch was sent out 7 November.
> It received reviews 8 November (after the start of the hard freeze).
> 
> The point of these freezes is that sometimes patches are wrong. And
> sometimes patches that look correct, are not correct. If we start
> making exceptions because "oh, it's trivial", that means we get these
> patches into the tree with much reduced time for anyone to catch any
> adverse effects before we make the stable tag. And at that point, the
> stable tag no longer has value.
> 
> (I am much more flexible on the topic of updating documentation, like
> Maintainers.txt, but even there we must be very careful.)

I haven't been following this specific patch, but now it does not look
like a bugfix to me. Without applying the patch, there is no bug
actually, functional or performance. The subject says, "Remove useless ...".

Optimizations, simplifications, refactorings, features, and so on, are
not bugfixes. They should not go in after the hard freeze. Even after
the soft freeze, they should only go in if the only remaining step is
the push (i.e. they should be ready for pushing before the soft freeze,
sufficiently reviewed.).

Thanks
Laszlo

>>>> -----Original Message-----
>>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>>> Sent: Friday, November 09, 2018 2:25 AM
>>>> To: Zeng, Star <star.zeng@intel.com>
>>>> Cc: Bi, Dandan <dandan.bi@intel.com>; edk2-devel@lists.01.org; Wu, Hao A
>>>> <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming
>>>> <liming.gao@intel.com>
>>>> Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless
>>>> NULL ptr check for NewPos
>>>>
>>>> On 8 November 2018 at 02:09, Zeng, Star <star.zeng@intel.com> wrote:
>>>>> Reviewed-by: Star Zeng <star.zeng@intel.com>
>>>>>
>>>>> -----Original Message-----
>>>>> From: Bi, Dandan
>>>>> Sent: Wednesday, November 7, 2018 10:53 PM
>>>>> To: edk2-devel@lists.01.org
>>>>> Cc: Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>;
>>>> Zeng, Star <star.zeng@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
>>>>> Subject: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr
>>>> check for NewPos
>>>>>
>>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1306
>>>>>
>>>>> In function UiDisplayMenu, the NewPos ptr which used to point to the
>>>> highlight menu entry. It will always point to the menu entry which need to be
>>>> highlighted or the gMenuOption menu if the highlight menu is not found.
>>>>> So we can remove the NULL ptr check for NewPos in this function.
>>>>> And add the ASSERT code to avoid if any false positive reports of NULL
>>>> pointer dereference issue raised from static analysis.
>>>>>
>>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>> Cc: Star Zeng <star.zeng@intel.com>
>>>>> Cc: Hao Wu <hao.a.wu@intel.com>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
>>>>
>>>> Why was this patch merged today? Surely, this doesn't meet the hard
>>>> freeze requirements ?
>>>>
>>>>> ---
>>>>>  MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c | 3 ++-
>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
>>>> b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
>>>>> index 7390f954b6..44f087fe01 100644
>>>>> --- a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
>>>>> +++ b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
>>>>> @@ -2880,10 +2880,11 @@ UiDisplayMenu (
>>>>>        //             MenuOption is set to NULL in Repaint
>>>>>        // NewPos:     Current menu option that need to hilight
>>>>>        //
>>>>>        ControlFlag = CfUpdateHelpString;
>>>>>
>>>>> +      ASSERT (NewPos != NULL);
>>>>>        UpdateHighlightMenuInfo(NewPos, TopOfScreen, SkipValue);
>>>>>
>>>>>        if (SkipHighLight) {
>>>>>          SkipHighLight = FALSE;
>>>>>          MenuOption    = SavedMenuOption;
>>>>> @@ -2908,11 +2909,11 @@ UiDisplayMenu (
>>>>>          Temp2 = SkipValue;
>>>>>        } else {
>>>>>          Temp2 = 0;
>>>>>        }
>>>>>
>>>>> -      if (NewPos != NULL && (MenuOption == NULL || NewPos !=
>>>> &MenuOption->Link)) {
>>>>> +      if (MenuOption == NULL || NewPos != &MenuOption->Link) {
>>>>>          if (MenuOption != NULL) {
>>>>>            //
>>>>>            // Remove the old highlight menu.
>>>>>            //
>>>>>            Status = DisplayOneMenu (MenuOption,
>>>>> --
>>>>> 2.18.0.windows.1
>>>>>
>>>>> _______________________________________________
>>>>> 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

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Gao, Liming]

Laszlo and Leif:
  I suggest to continue to update wiki page with more information. If so, we can avoid such case again. 

  For this change, it has no real functionality impact. If you think roll back is better than keep it, I am OK. 

Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Friday, November 9, 2018 7:02 PM
> To: Leif Lindholm <leif.lindholm@linaro.org>; Gao, Liming <liming.gao@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos
> 
> On 11/09/18 11:49, Leif Lindholm wrote:
> > On Fri, Nov 09, 2018 at 07:56:07AM +0100, Ard Biesheuvel wrote:
> >> On 9 November 2018 at 01:19, Gao, Liming <liming.gao@intel.com> wrote:
> >>> Ard:
> >>>   This is a small fix. And, this patch is sent before the hard
> >>>   freeze. It is the low risk for this release. So, I push it.
> >>
> >> OK, fair enough.
> >
> > I don't agree actually.
> >
> > https://github.com/tianocore/tianocore.github.io/wiki/HardFeatureFreeze
> > specifies clearly that only bug fixes are permitted in during hard
> > freeze. Maybe we could document that a bit more explicitly, but this
> > patch was no bugfix. It should not have gone in.
> >
> > By my interpretation, it would not even fulfill the requirements for
> > https://github.com/lersek/edk2/wiki/SoftFeatureFreeze:
> > "By the date of the soft feature freeze, developers must have sent
> > their patches to the mailing list and received positive maintainer
> > reviews."
> > Soft feature freeze was 1 November. The patch was sent out 7 November.
> > It received reviews 8 November (after the start of the hard freeze).
> >
> > The point of these freezes is that sometimes patches are wrong. And
> > sometimes patches that look correct, are not correct. If we start
> > making exceptions because "oh, it's trivial", that means we get these
> > patches into the tree with much reduced time for anyone to catch any
> > adverse effects before we make the stable tag. And at that point, the
> > stable tag no longer has value.
> >
> > (I am much more flexible on the topic of updating documentation, like
> > Maintainers.txt, but even there we must be very careful.)
> 
> I haven't been following this specific patch, but now it does not look
> like a bugfix to me. Without applying the patch, there is no bug
> actually, functional or performance. The subject says, "Remove useless ...".
> 
> Optimizations, simplifications, refactorings, features, and so on, are
> not bugfixes. They should not go in after the hard freeze. Even after
> the soft freeze, they should only go in if the only remaining step is
> the push (i.e. they should be ready for pushing before the soft freeze,
> sufficiently reviewed.).
> 
> Thanks
> Laszlo
> 
> >>>> -----Original Message-----
> >>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >>>> Sent: Friday, November 09, 2018 2:25 AM
> >>>> To: Zeng, Star <star.zeng@intel.com>
> >>>> Cc: Bi, Dandan <dandan.bi@intel.com>; edk2-devel@lists.01.org; Wu, Hao A
> >>>> <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming
> >>>> <liming.gao@intel.com>
> >>>> Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless
> >>>> NULL ptr check for NewPos
> >>>>
> >>>> On 8 November 2018 at 02:09, Zeng, Star <star.zeng@intel.com> wrote:
> >>>>> Reviewed-by: Star Zeng <star.zeng@intel.com>
> >>>>>
> >>>>> -----Original Message-----
> >>>>> From: Bi, Dandan
> >>>>> Sent: Wednesday, November 7, 2018 10:53 PM
> >>>>> To: edk2-devel@lists.01.org
> >>>>> Cc: Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> >>>> Zeng, Star <star.zeng@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> >>>>> Subject: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr
> >>>> check for NewPos
> >>>>>
> >>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1306
> >>>>>
> >>>>> In function UiDisplayMenu, the NewPos ptr which used to point to the
> >>>> highlight menu entry. It will always point to the menu entry which need to be
> >>>> highlighted or the gMenuOption menu if the highlight menu is not found.
> >>>>> So we can remove the NULL ptr check for NewPos in this function.
> >>>>> And add the ASSERT code to avoid if any false positive reports of NULL
> >>>> pointer dereference issue raised from static analysis.
> >>>>>
> >>>>> Cc: Liming Gao <liming.gao@intel.com>
> >>>>> Cc: Eric Dong <eric.dong@intel.com>
> >>>>> Cc: Star Zeng <star.zeng@intel.com>
> >>>>> Cc: Hao Wu <hao.a.wu@intel.com>
> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>>>> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> >>>>
> >>>> Why was this patch merged today? Surely, this doesn't meet the hard
> >>>> freeze requirements ?
> >>>>
> >>>>> ---
> >>>>>  MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c | 3 ++-
> >>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> >>>> b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> >>>>> index 7390f954b6..44f087fe01 100644
> >>>>> --- a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> >>>>> +++ b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> >>>>> @@ -2880,10 +2880,11 @@ UiDisplayMenu (
> >>>>>        //             MenuOption is set to NULL in Repaint
> >>>>>        // NewPos:     Current menu option that need to hilight
> >>>>>        //
> >>>>>        ControlFlag = CfUpdateHelpString;
> >>>>>
> >>>>> +      ASSERT (NewPos != NULL);
> >>>>>        UpdateHighlightMenuInfo(NewPos, TopOfScreen, SkipValue);
> >>>>>
> >>>>>        if (SkipHighLight) {
> >>>>>          SkipHighLight = FALSE;
> >>>>>          MenuOption    = SavedMenuOption;
> >>>>> @@ -2908,11 +2909,11 @@ UiDisplayMenu (
> >>>>>          Temp2 = SkipValue;
> >>>>>        } else {
> >>>>>          Temp2 = 0;
> >>>>>        }
> >>>>>
> >>>>> -      if (NewPos != NULL && (MenuOption == NULL || NewPos !=
> >>>> &MenuOption->Link)) {
> >>>>> +      if (MenuOption == NULL || NewPos != &MenuOption->Link) {
> >>>>>          if (MenuOption != NULL) {
> >>>>>            //
> >>>>>            // Remove the old highlight menu.
> >>>>>            //
> >>>>>            Status = DisplayOneMenu (MenuOption,
> >>>>> --
> >>>>> 2.18.0.windows.1
> >>>>>
> >>>>> _______________________________________________
> >>>>> 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
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Leif Lindholm]

On Fri, Nov 09, 2018 at 03:32:03PM +0000, Gao, Liming wrote:
> Laszlo and Leif:
>   I suggest to continue to update wiki page with more
>   information. If so, we can avoid such case again.

Agreed, we need to be able to interpret what the process says
identically.

>   For this change, it has no real functionality impact.

But this is my point: we should not be making judgement calls this
late in the process. If I look at that patch, sure, it looks fine to
me. I still don't want it going in during freeze, because I don't
_know_ it has no real functionality impact. I may be missing
something. And even if I am not missing anything, the reshuffle may
still be sufficient to change compiler behaviour, exposing a
previously undetected bug.

>   If you think roll back is better than keep it, I am OK.

That would be my preference. I have zero objection to it going back in
immediately after the stable tag is made.

Regards,

Leif

> Thanks
> Liming
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> > Sent: Friday, November 9, 2018 7:02 PM
> > To: Leif Lindholm <leif.lindholm@linaro.org>; Gao, Liming <liming.gao@intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> > Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos
> > 
> > On 11/09/18 11:49, Leif Lindholm wrote:
> > > On Fri, Nov 09, 2018 at 07:56:07AM +0100, Ard Biesheuvel wrote:
> > >> On 9 November 2018 at 01:19, Gao, Liming <liming.gao@intel.com> wrote:
> > >>> Ard:
> > >>>   This is a small fix. And, this patch is sent before the hard
> > >>>   freeze. It is the low risk for this release. So, I push it.
> > >>
> > >> OK, fair enough.
> > >
> > > I don't agree actually.
> > >
> > > https://github.com/tianocore/tianocore.github.io/wiki/HardFeatureFreeze
> > > specifies clearly that only bug fixes are permitted in during hard
> > > freeze. Maybe we could document that a bit more explicitly, but this
> > > patch was no bugfix. It should not have gone in.
> > >
> > > By my interpretation, it would not even fulfill the requirements for
> > > https://github.com/lersek/edk2/wiki/SoftFeatureFreeze:
> > > "By the date of the soft feature freeze, developers must have sent
> > > their patches to the mailing list and received positive maintainer
> > > reviews."
> > > Soft feature freeze was 1 November. The patch was sent out 7 November.
> > > It received reviews 8 November (after the start of the hard freeze).
> > >
> > > The point of these freezes is that sometimes patches are wrong. And
> > > sometimes patches that look correct, are not correct. If we start
> > > making exceptions because "oh, it's trivial", that means we get these
> > > patches into the tree with much reduced time for anyone to catch any
> > > adverse effects before we make the stable tag. And at that point, the
> > > stable tag no longer has value.
> > >
> > > (I am much more flexible on the topic of updating documentation, like
> > > Maintainers.txt, but even there we must be very careful.)
> > 
> > I haven't been following this specific patch, but now it does not look
> > like a bugfix to me. Without applying the patch, there is no bug
> > actually, functional or performance. The subject says, "Remove useless ...".
> > 
> > Optimizations, simplifications, refactorings, features, and so on, are
> > not bugfixes. They should not go in after the hard freeze. Even after
> > the soft freeze, they should only go in if the only remaining step is
> > the push (i.e. they should be ready for pushing before the soft freeze,
> > sufficiently reviewed.).
> > 
> > Thanks
> > Laszlo
> > 
> > >>>> -----Original Message-----
> > >>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > >>>> Sent: Friday, November 09, 2018 2:25 AM
> > >>>> To: Zeng, Star <star.zeng@intel.com>
> > >>>> Cc: Bi, Dandan <dandan.bi@intel.com>; edk2-devel@lists.01.org; Wu, Hao A
> > >>>> <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming
> > >>>> <liming.gao@intel.com>
> > >>>> Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless
> > >>>> NULL ptr check for NewPos
> > >>>>
> > >>>> On 8 November 2018 at 02:09, Zeng, Star <star.zeng@intel.com> wrote:
> > >>>>> Reviewed-by: Star Zeng <star.zeng@intel.com>
> > >>>>>
> > >>>>> -----Original Message-----
> > >>>>> From: Bi, Dandan
> > >>>>> Sent: Wednesday, November 7, 2018 10:53 PM
> > >>>>> To: edk2-devel@lists.01.org
> > >>>>> Cc: Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> > >>>> Zeng, Star <star.zeng@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> > >>>>> Subject: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr
> > >>>> check for NewPos
> > >>>>>
> > >>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1306
> > >>>>>
> > >>>>> In function UiDisplayMenu, the NewPos ptr which used to point to the
> > >>>> highlight menu entry. It will always point to the menu entry which need to be
> > >>>> highlighted or the gMenuOption menu if the highlight menu is not found.
> > >>>>> So we can remove the NULL ptr check for NewPos in this function.
> > >>>>> And add the ASSERT code to avoid if any false positive reports of NULL
> > >>>> pointer dereference issue raised from static analysis.
> > >>>>>
> > >>>>> Cc: Liming Gao <liming.gao@intel.com>
> > >>>>> Cc: Eric Dong <eric.dong@intel.com>
> > >>>>> Cc: Star Zeng <star.zeng@intel.com>
> > >>>>> Cc: Hao Wu <hao.a.wu@intel.com>
> > >>>>> Contributed-under: TianoCore Contribution Agreement 1.1
> > >>>>> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> > >>>>
> > >>>> Why was this patch merged today? Surely, this doesn't meet the hard
> > >>>> freeze requirements ?
> > >>>>
> > >>>>> ---
> > >>>>>  MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c | 3 ++-
> > >>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >>>>>
> > >>>>> diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> > >>>> b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> > >>>>> index 7390f954b6..44f087fe01 100644
> > >>>>> --- a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> > >>>>> +++ b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> > >>>>> @@ -2880,10 +2880,11 @@ UiDisplayMenu (
> > >>>>>        //             MenuOption is set to NULL in Repaint
> > >>>>>        // NewPos:     Current menu option that need to hilight
> > >>>>>        //
> > >>>>>        ControlFlag = CfUpdateHelpString;
> > >>>>>
> > >>>>> +      ASSERT (NewPos != NULL);
> > >>>>>        UpdateHighlightMenuInfo(NewPos, TopOfScreen, SkipValue);
> > >>>>>
> > >>>>>        if (SkipHighLight) {
> > >>>>>          SkipHighLight = FALSE;
> > >>>>>          MenuOption    = SavedMenuOption;
> > >>>>> @@ -2908,11 +2909,11 @@ UiDisplayMenu (
> > >>>>>          Temp2 = SkipValue;
> > >>>>>        } else {
> > >>>>>          Temp2 = 0;
> > >>>>>        }
> > >>>>>
> > >>>>> -      if (NewPos != NULL && (MenuOption == NULL || NewPos !=
> > >>>> &MenuOption->Link)) {
> > >>>>> +      if (MenuOption == NULL || NewPos != &MenuOption->Link) {
> > >>>>>          if (MenuOption != NULL) {
> > >>>>>            //
> > >>>>>            // Remove the old highlight menu.
> > >>>>>            //
> > >>>>>            Status = DisplayOneMenu (MenuOption,
> > >>>>> --
> > >>>>> 2.18.0.windows.1
> > >>>>>
> > >>>>> _______________________________________________
> > >>>>> 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
> > 
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Laszlo Ersek]

On 11/09/18 17:01, Leif Lindholm wrote:
> On Fri, Nov 09, 2018 at 03:32:03PM +0000, Gao, Liming wrote:
>> Laszlo and Leif:
>>   I suggest to continue to update wiki page with more
>>   information. If so, we can avoid such case again.
> 
> Agreed, we need to be able to interpret what the process says
> identically.

Making the wiki real precise on this question requires dedicated work. I
was OK to simply send a patch about the announcements, but this topic
requires more thinking, more discussion, and well, more tracking.

Mike: would it be proper to create a "Wiki" Component under the EDK2
product in the bugzilla?

If so, I would suggest filing a BZ against that (new) component, and
then we should discuss the freeze scopes in one of the next community
meetings.

Thanks
Laszlo

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Kinney, Michael D]

Hi Laszlo,

There is already a "Documentation" component under
EDK2 in BZ.  Can we use that and identify the target
of the documentation change is in a Wiki page?

When you select "Documentation" there is a drop down
for "Tianocore documents" that lists the documents
published through GitBook along with a catch all called
"Other Document".  Is this the drop down you would like
to see "Wiki" added?

Thanks,

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, November 9, 2018 8:33 AM
> To: Leif Lindholm <leif.lindholm@linaro.org>; Gao,
> Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Cetola, Stephano
> <stephano.cetola@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine:
> Remove useless NULL ptr check for NewPos
> 
> On 11/09/18 17:01, Leif Lindholm wrote:
> > On Fri, Nov 09, 2018 at 03:32:03PM +0000, Gao, Liming
> wrote:
> >> Laszlo and Leif:
> >>   I suggest to continue to update wiki page with
> more
> >>   information. If so, we can avoid such case again.
> >
> > Agreed, we need to be able to interpret what the
> process says
> > identically.
> 
> Making the wiki real precise on this question requires
> dedicated work. I
> was OK to simply send a patch about the announcements,
> but this topic
> requires more thinking, more discussion, and well, more
> tracking.
> 
> Mike: would it be proper to create a "Wiki" Component
> under the EDK2
> product in the bugzilla?
> 
> If so, I would suggest filing a BZ against that (new)
> component, and
> then we should discuss the freeze scopes in one of the
> next community
> meetings.
> 
> Thanks
> Laszlo

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Laszlo Ersek]

On 11/09/18 18:14, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> There is already a "Documentation" component under
> EDK2 in BZ.  Can we use that and identify the target
> of the documentation change is in a Wiki page?

Absolutely.

> When you select "Documentation" there is a drop down
> for "Tianocore documents" that lists the documents
> published through GitBook along with a catch all called
> "Other Document".  Is this the drop down you would like
> to see "Wiki" added?

I was undecided. I recalled that earlier there used to be a "TianoCore
Website" component somewhere, but now I couldn't find it. Also, I
considered both options we're discussing now (i.e., EDK2|Wiki, vs.
EDK2|Documentation|Wiki). I couldn't really decide.
"EDK2|Documentation|Xxx" seemed to suggest serious specifications that
we publish "in print".

I think I'd be fine with either option (EDK2|Wiki vs.
EDK2|Documentation|Wiki); whichever is easier to implement on the
Bugzilla side. :)

Thank you!
Laszlo

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Kinney, Michael D]

Laszlo,

I added EDK2|Documentation|Wiki Pages

I also updated the sort order so Wiki Pages is
first, followed by maintained GitBook documents
sorted alphabetically, with "Other Document" last.

Thanks,

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, November 9, 2018 9:24 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> Leif Lindholm <leif.lindholm@linaro.org>; Gao, Liming
> <liming.gao@intel.com>; Cetola, Stephano
> <stephano.cetola@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine:
> Remove useless NULL ptr check for NewPos
> 
> On 11/09/18 18:14, Kinney, Michael D wrote:
> > Hi Laszlo,
> >
> > There is already a "Documentation" component under
> > EDK2 in BZ.  Can we use that and identify the target
> > of the documentation change is in a Wiki page?
> 
> Absolutely.
> 
> > When you select "Documentation" there is a drop down
> > for "Tianocore documents" that lists the documents
> > published through GitBook along with a catch all
> called
> > "Other Document".  Is this the drop down you would
> like
> > to see "Wiki" added?
> 
> I was undecided. I recalled that earlier there used to
> be a "TianoCore
> Website" component somewhere, but now I couldn't find
> it. Also, I
> considered both options we're discussing now (i.e.,
> EDK2|Wiki, vs.
> EDK2|Documentation|Wiki). I couldn't really decide.
> "EDK2|Documentation|Xxx" seemed to suggest serious
> specifications that
> we publish "in print".
> 
> I think I'd be fine with either option (EDK2|Wiki vs.
> EDK2|Documentation|Wiki); whichever is easier to
> implement on the
> Bugzilla side. :)
> 
> Thank you!
> Laszlo

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Laszlo Ersek]

On 11/09/18 18:49, Kinney, Michael D wrote:
> Laszlo,
> 
> I added EDK2|Documentation|Wiki Pages
> 
> I also updated the sort order so Wiki Pages is
> first, followed by maintained GitBook documents
> sorted alphabetically, with "Other Document" last.

Thanks, Mike! I've filed
<https://bugzilla.tianocore.org/show_bug.cgi?id=1320>, and assigned it
to Stephano.

Stephano, I hope that's alright with you -- with that assignment I
attempted to express that the topic should be brought to the community.

Thanks
Laszlo

Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[stephano]

On 11/9/2018 12:07 PM, Laszlo Ersek wrote:
> On 11/09/18 18:49, Kinney, Michael D wrote:
>> Laszlo,
>>
>> I added EDK2|Documentation|Wiki Pages
>>
>> I also updated the sort order so Wiki Pages is
>> first, followed by maintained GitBook documents
>> sorted alphabetically, with "Other Document" last.
> 
> Thanks, Mike! I've filed
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1320>, and assigned it
> to Stephano.
> 
> Stephano, I hope that's alright with you -- with that assignment I
> attempted to express that the topic should be brought to the community.

Yes, that is fine. I would even suggest making me the default owner for 
any documentation category bugs. I can work with Kevin Shaw on any of 
these types of issues and hopefully take these off Mike's plate.