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-Format > > 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, 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. 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. 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) { >> -- >> 1.8.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 ------------------------------------------------------------------------------ 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
