Of all the gin joints in all the towns in all the world, Jordan Justen had to walk into mine at 00:53:23 on Friday 10 July 2015 and say:
> On 2015-07-10 00:18:15, Laszlo Ersek wrote: > > Ooops :) > > > > Apparently I was too quick to commit this, without waiting for your > > review. Anyway, some comments: > > > > On 07/10/15 09:05, Jordan Justen wrote: > > > Patch subject doesn't contain a package/module prefix. I suggest: > > > > > > OvmfPkg/QemuFwCfgLib: Avoid "variable set but not used" warning from > > > GCC > > > > I fixed that up when committing. > > > > > https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Fo > > > rmat For the record, I searched for the patch submission rules, but I must not have specified the exact right search terms to bring up this page because I never saw it. I did read the Contributions.txt file, but the instructions it contains are not the same as what's on the URL shown above, and they don't place special emphasis on the module name. Also, neither document mentions that you must include a CC list of maintainers. Given that Laszlo asked for the patch in the first place, it did not occur to me this would be necessary. If you want all patches to adhere to a rigid pattern, every time, then I suggest you a) put the exact set of rules in a file in the top level of the source tree, with fully documented examples and/or b) include a script to generate the submissions that prompts the user to enter all required fields. Be aware too that not every project uses the exact same rules for patch submissions, so don't be surprised if new (or infrequent) contributors can't immediately decipher your particular secret handshake. > > > On 2015-07-09 17:09:20, Bill Paul wrote: > > >> The FileReserved variable in QemuFwCfgFindFile() is only used to skip > > >> over the reserved field in file headers, which causes newer versions > > >> of GCC to flag it with a "variable set but not used" warning (which > > >> is normally not visible since as of right now these warnings are > > >> supressed). It's true that the value read into FileReserved is never > > >> used, but this is intentional. This patch adds a do-nothing reference > > >> to silence the warning. > > >> > > >> Contributed-under: TianoCore Contribution Agreement 1.0 > > >> Signed-off-by: Bill Paul <wp...@windriver.com> > > > > > > You should add Cc's here (package owners are in Maintainers.txt): > > > > > > Cc: Jordan Justen <jordan.l.jus...@intel.com> > > > Cc: Laszlo Ersek <ler...@redhat.com> > > > > Agree, but ultimately both of us noticed the patch :) > > > > >> --- > > >> > > >> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 1 + > > >> 1 file changed, 1 insertion(+) > > >> > > >> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > > >> b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > > >> index 24424f8..573d90f 100644 > > >> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > > >> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > > >> @@ -283,6 +283,7 @@ QemuFwCfgFindFile ( > > >> > > >> FileSize = QemuFwCfgRead32 (); > > >> FileSelect = QemuFwCfgRead16 (); > > >> FileReserved = QemuFwCfgRead16 (); > > >> > > >> + (VOID) FileReserved; /* Force a do-nothing reference. */ > > > > > > We use '//' comments in code. > > > > > > Coding Standards Spec, Section "5. 4.2 Internal Comments": > > > > > > "For internal code comments, use C++ style (//) comment lines." > > > > Yes, I did not miss that, > > Well, I guess you could have tweaked it youself rather than requiring > a repost. (Like the patch subject.) Let me be clear that it doesn't matter me what any coding convention says, it doesn't matter that the C spec allows it, and it doesn't matter to me how many might advocate it, what arguments they put forth to support it or in how much esteem they are held: I will never, ever, EVER use C++-style comments in C code. > > but I thought this was really minor here, and > > I was happy that Bill finally decided to submit a patch! :) I didn't > > want to discourage further contributions from him by splitting hairs > > *here* :) > > > > > Instead of this change, why not just remove the FileReserved variable > > > > > > and change the code: > > > // > > > // Read 2 reserved bytes > > > // > > > QemuFwCfgRead16 (); > > > > I named that option before, in a slightly different form: > > (VOID) QemuFwCfgRead16 (); > > > > Because, without the explicit cast to VOID, some compiler might complain > > about the return value being ignored. > > That can't be true. Is it?? I can't imagine any significantly sized C > code base would not generate that warning. Recent versions of GCC will warn about an unused return value if you tag a function with __attribute__((warn_unused_result)) (unless you use -Wno-unused- result to suppress it). There are also static analysis tools that do complain about this. Our own coding standard mandates that you use a (void) cast for any function call where the function is not void and the return value is intentionally ignored, including printf(). I think this is intended to encourage people to handle error cases more often. It's debatable whether or not it actually has that effect. > Just below this code in QemuFwCfgS3Enabled we ignore the returns from > QemuFwCfgSelectItem and QemuFwCfgReadBytes. Uh... QemuFwCfgSelectItem() and QemuFwCfgReadBytes() are declared as VOID. They don't return anything. > > In any case, I called Bill's version (the one I ultimately committed > > too) more readable, so if you disagree with that, then it's my fault. > > Declare the variable, set it, then actively ignore it vs just not > having the variable... The current code format not only makes it obvious to the reader just why the value must be read but discarded, it also makes it easier to update the code in the future if that reserved field is eventually put into use. ("There will one day be lemon-soaked paper napkins...") I must confess that preserving this information was not my original motive for changing things as I did -- I was just using the "shortest distance" rule -- but Laszlo's rationale seemed reasonable to me too. In any case, I will let you guys fuss over the color of the bike shed. -Bill > -Jordan > > > If you'd like, you can update the style in the source and commit it at > > once as a separate patch, and add my R-b immediately (based on the > > above). > > > > Thanks > > Laszlo > > > > > -Jordan > > > > > >> InternalQemuFwCfgReadBytes (sizeof (FName), FName); > > >> > > >> if (AsciiStrCmp (Name, FName) == 0) { > > >> > > >> ---------------------------------------------------------------------- > > >> -------- Don't Limit Your Business. Reach for the Cloud. > > >> GigeNET's Cloud Solutions provide you with the tools and support that > > >> you need to offload your IT needs and focus on growing your business. > > >> Configured For All Businesses. Start Your Cloud Today. > > >> https://www.gigenetcloud.com/ > > >> _______________________________________________ > > >> edk2-devel mailing list > > >> edk2-devel@lists.sourceforge.net > > >> https://lists.sourceforge.net/lists/listinfo/edk2-devel -- ============================================================================= -Bill Paul (510) 749-2329 | Senior Member of Technical Staff, wp...@windriver.com | Master of Unix-Fu - Wind River Systems ============================================================================= "I put a dollar in a change machine. Nothing changed." - George Carlin ============================================================================= ------------------------------------------------------------------------------ Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
