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-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,
Well, I guess you could have tweaked it youself rather than requiring
a repost. (Like the patch subject.)
> 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.
Just below this code in QemuFwCfgS3Enabled we ignore the returns from
QemuFwCfgSelectItem and QemuFwCfgReadBytes.
> 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...
-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) {
> >> --
> >> 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
