Re: [edk2] Git patch commit messages - Re: [edk2-buildtools] [Patch] Remove GCC option -Wno-missing-braces option

Subject: Re: [edk2] Git patch commit messages - Re: [edk2-buildtools] [Patch] Remove GCC option -Wno-missing-braces option

From: "Gao, Liming" <liming.gao@intel.com>

To: Jordan Justen <jljusten@gmail.com>, Laszlo Ersek <lersek@redhat.com>

Date: 2014-06-25 13:02:29

Got it.

I just commit BaseTools trunk change at 2670. 

Thanks
Liming
-----Original Message-----
From: Jordan Justen [mailto:jljusten@gmail.com] 
Sent: Wednesday, June 25, 2014 11:49 AM
To: Gao, Liming; Laszlo Ersek
Cc: Olivier Martin; edk2-devel@lists.sourceforge.net; edk2-buildtools-devel@lists.sourceforge.net
Subject: Git patch commit messages - Re: [edk2-buildtools] [edk2] [Patch] Remove GCC option -Wno-missing-braces option

On Tue, Jun 24, 2014 at 7:02 PM, Gao, Liming  wrote:
> Lzalo:
>   For your patch for OVMF platform, could you provide the commit log message to me? Then, I will help commit it. Last, I commit this BaseTools Conf change.

Liming,

Laszlo is wisely a git user. :) Therefore his patch included a commit message. If you open 0004-OvmfPkg-add-missing-braces-to-aggregate-and-or-union.patch, then you will see:

---
Subject: [PATCH 4/4] OvmfPkg: add missing braces to aggregate and/or union  initializers

Lack of these braces causes build errors when -Wno-missing-braces is absent. Spelling out more braces also helps understanding the code.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---

git puts the first line of the commit message as the subject. This helps because when git send-email is used, then people have a good idea of what the patch is about from the subject of the email.

So, Laszlo's commit message was:
===
OvmfPkg: add missing braces to aggregate and/or union initializers

Lack of these braces causes build errors when -Wno-missing-braces is absent. Spelling out more braces also helps understanding the code.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek  ===

The git command 'git am' will take an email message (or git patch attached to an email), and apply the patch along with the commit message making it very easy for the patch submitter to send the patch along with a good commit message, and also making it very easy for the maintainer to incorporate that patch into the tree.

Laszlo,

Your 0004 patch is Reviewed-by: Jordan Justen , and committed. I think it is usually better to use git send-email with patches, rather than attaching them because they are more easy to notice and to code review.

Thanks for the contribution!

-Jordan

> -----Original Message-----
> From: Gao, Liming [mailto:liming.gao@intel.com]
> Sent: Monday, June 23, 2014 1:16 PM
> To: Olivier Martin; edk2-devel@lists.sourceforge.net; 
> edk2-buildtools-devel@lists.sourceforge.net
> Subject: Re: [edk2-buildtools] [edk2] [Patch] Remove GCC option 
> -Wno-missing-braces option
>
> Lzalo:
>   This patch includes the change to *_ELFGCC_X64_CC_FLAGS. But, no changes is for XCLANG and XCODE. If Andrew agrees this change, I can apply it in XCLANG and XCODE tool chain.
>
>   4) I only verify it in common package. Thanks for your test on other 
> platforms. Your patch for OVMF platform is good to me. Reviewed-by: 
> Gao, Liming 
>
> Thanks
> Liming
> -----Original Message-----
> From: Olivier Martin [mailto:olivier.martin@arm.com]
> Sent: Friday, June 20, 2014 9:30 PM
> To: edk2-devel@lists.sourceforge.net; Gao, Liming; 
> edk2-buildtools-devel@lists.sourceforge.net
> Subject: RE: [edk2] [edk2-buildtools] [Patch] Remove GCC option 
> -Wno-missing-braces option
>
> Thanks Lazlo, I was actually doing the same exercise with our build system.
> I am still going through them...
>
> But as far as I have seen, there is no error int the common packages. So the patch looks fine to me.
>
> Reviewed-By: Olivier Martin 
> Tested-By: Olivier Martin 
>
>
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: 20 June 2014 14:17
>> To: Gao, Liming; edk2-buildtools-devel@lists.sourceforge.net; edk2- 
>> devel@lists.sourceforge.net
>> Subject: Re: [edk2] [edk2-buildtools] [Patch] Remove GCC option -Wno- 
>> missing-braces option
>>
>> On 06/20/14 12:00, Gao, Liming wrote:
>> > Hi, all
>> >
>> >   -Wno-missing-braces option is used to disable warning when the 
>> > initialized value without braces is set into structure. To remove
>> this
>> > option can detect such wrong case in source code. This patch remove
>> it
>> > from the default GCC compiler option in 
>> > BaseTools/Conf/tools_def.template. Please help review it.
>>
>> (1) The patch makes sense to me, because a human should arguably 
>> prefer
>>
>>                    int b[2][2] = { { 0, 1 }, { 2, 3 } };
>>
>> over
>>
>>                    int a[2][2] = { 0, 1, 2, 3 };
>>
>> It could be a little inconvenient for machine generated code though.
>> (Especially that the warning will be treated as an error.) But I 
>> guess it should be fine.
>>
>> (2) The patch removes the flag from two GCC defines (GCC_ALL_CC_FLAGS 
>> and GCC44_ALL_CC_FLAGS). In my edk2 clone there's a third, similar
>> flag:
>> "*_ELFGCC_X64_CC_FLAGS". Did you leave that out deliberately?
>>
>> (3) Other compilers seem to retain -Wno-missing-braces (eg. XCLANG, 
>> XCODE). Is that intentional, or are they not modified only because 
>> it's hard to test them? (I don't mind if the latter is the case, I'd 
>> just like to understand.)
>>
>> (4) In order to test the patch, I grabbed my existing 
>> "Conf/tools_def.txt" and removed all instances of -Wno-missing-braces.
>>
>> Then I tried to build OVMF (for both Ia32 and X64), and also
>> (cross-build) "ArmVExpress-FVP-AArch64" and 
>> "ArmVExpress-RTSM-AEMv8Ax4-foundation".
>>
>> The change triggers a few build errors in OVMF, and many more in the 
>> ARM packages. Please see my attached fixes. I propose that these 
>> patches be applied first (after review), before committing & syncing 
>> this BaseTools change.
>>
>> Thanks
>> Laszlo
>
>
>
>
> ----------------------------------------------------------------------
> -------- HPCC Systems Open Source Big Data Platform from LexisNexis 
> Risk Solutions Find What Matters Most in Your Big Data with HPCC Systems Open Source. Fast. Scalable. Simple. Ideal for Dirty Data.
> Leverages Graph Analysis for Fast Processing & Easy Data Exploration 
> http://p.sf.net/sfu/hpccsystems 
> _______________________________________________
> edk2-buildtools-devel mailing list
> edk2-buildtools-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-buildtools-devel
>
> ----------------------------------------------------------------------
> -------- Open source business process management suite built on Java 
> and Eclipse Turn processes into business applications with Bonita BPM 
> Community Edition Quickly connect people, data, and systems into 
> organized workflows Winner of BOSSIE, CODIE, OW2 and Gartner awards 
> http://p.sf.net/sfu/Bonitasoft 
> _______________________________________________
> edk2-buildtools-devel mailing list
> edk2-buildtools-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-buildtools-devel
------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel