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:[email protected]] On Behalf Of > > Laszlo Ersek > > Sent: Friday, November 9, 2018 7:02 PM > > To: Leif Lindholm <[email protected]>; Gao, Liming > > <[email protected]> > > Cc: Kinney, Michael D <[email protected]>; [email protected] > > 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 <[email protected]> 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:[email protected]] > > >>>> Sent: Friday, November 09, 2018 2:25 AM > > >>>> To: Zeng, Star <[email protected]> > > >>>> Cc: Bi, Dandan <[email protected]>; [email protected]; Wu, Hao > > >>>> A > > >>>> <[email protected]>; Dong, Eric <[email protected]>; Gao, Liming > > >>>> <[email protected]> > > >>>> Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless > > >>>> NULL ptr check for NewPos > > >>>> > > >>>> On 8 November 2018 at 02:09, Zeng, Star <[email protected]> wrote: > > >>>>> Reviewed-by: Star Zeng <[email protected]> > > >>>>> > > >>>>> -----Original Message----- > > >>>>> From: Bi, Dandan > > >>>>> Sent: Wednesday, November 7, 2018 10:53 PM > > >>>>> To: [email protected] > > >>>>> Cc: Gao, Liming <[email protected]>; Dong, Eric > > >>>>> <[email protected]>; > > >>>> Zeng, Star <[email protected]>; Wu, Hao A <[email protected]> > > >>>>> 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 <[email protected]> > > >>>>> Cc: Eric Dong <[email protected]> > > >>>>> Cc: Star Zeng <[email protected]> > > >>>>> Cc: Hao Wu <[email protected]> > > >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 > > >>>>> Signed-off-by: Dandan Bi <[email protected]> > > >>>> > > >>>> 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 > > >>>>> [email protected] > > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel > > >> _______________________________________________ > > >> edk2-devel mailing list > > >> [email protected] > > >> https://lists.01.org/mailman/listinfo/edk2-devel > > > > _______________________________________________ > > edk2-devel mailing list > > [email protected] > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
