On 2015-06-04 10:47:37, Laszlo Ersek wrote: > Jiewen, > > On 06/04/15 16:34, Yao, Jiewen wrote:> Hi > > > > Here is patch to support UEFI2.5, chapter 4, section 4.6 Properties > > Table feature. > > > > MdePkg \u2013 add Properties table definition. > > MdeModulePkg \u2013 add properties table support in DXE core. > > MdeModulePkg \u2013 add PropertiesTableAttributesDxe driver to set > > ACPINvs/Reserved memory type to be XP. > > > > Signed-off-by: Yao, Jiewen jiewen....@intel.com > > <mailto:jiewen....@intel.com> > > Reviewed-by: Zeng, Star star.z...@intel.com <mailto:star.z...@intel.com> > > Reviewed-by: Gao, Liming liming....@intel.com <mailto:liming....@intel.com> > > Honest question: do you ever intend to abandon the following bad > practices: > - extremely long lines, > - huge code bombs in a few patches, > - posting patches as attachments? > > The diffstat for the second attachment is: > > /PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.uni > | 1 > /PropertiesTableAttributesDxe/PropertiesTableAttributesDxeExtra.uni > | 1 > Core/Dxe/DxeMain.h > | 29 > Core/Dxe/DxeMain.inf > | 4 > Core/Dxe/DxeMain/DxeMain.c > | 2 > Core/Dxe/Image/Image.c > | 2 > Core/Dxe/Misc/PropertiesTable.c > | 1418 ++++++++++ > MdeModulePkg.dec > | 22 > MdeModulePkg.dsc > | 2 > Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.c > | 412 ++ > Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.inf > | 114 > Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.uni > | 1 > Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxeExtra.uni > | 1 > 13 files changed, 2009 insertions(+) > > Also, the longest line in the patch is 214 characters. > > Do you ever intend to adopt inline patches, to break up features into > series of patches, and to follow the line length hints of the edk2 > coding style? > > Honestly, I have the impression about these postings that they are an > after-the-fact (non-)courtesy to the mailing list. "We wrote this at > Intel, it has been reviewed, it's going in, we'll ignore your formatting > requests that you've repeated many times; if you are unable to review it > as-is, your loss". > > If you are committed to ignore these bugs in your patches in the long > term, I'd like to know that.
Jiewen, it seems like Laszlo expressed some (valid) concerns with your patch, but you didn't respond, and checked them in anyway. Considering how much effort it is for people to code review patches, it is not very friendly to ignore their feedback. Occasionally, in urgent cases, we can disagree and still move forward even when there is disagreement, but I don't think it is good to ignore the discussion entirely and move forward with no regard for the concerns. Did you intend this edk2-devel "code-review" to actually be "We wrote this at Intel, it has been reviewed, it's going in" as was Laszlo's concern? I think the real start of the issue is that this should have been posted to edk2-devel before there was any Reviewed-by. And, following the posting, all feedback from the community should be considered. Mike, do you have any thoughts on how we might be able to address these issues? Maybe we could consider removing commit privileges for developers that don't follow the process? Is there someone else on the team besides you that I should ask this question to? Thanks, -Jordan ------------------------------------------------------------------------------ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
