On 2015-07-10 10:46:19, Bill Paul wrote:
> 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.
True, but we are trying to follow some fairly common practices.
You'd like us to just accept your contributions as is, and not provide
any feedback? I don't think that review feedback amounts to a 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.
You shouldn't expect a project to be receptive to your contributions
if you are hostile towards its coding standards. What would be your
reaction if someone submitted a patch to your project with c++
comments in a C file?
I would think it is more constructive to try to open a discussion on
having the coding standards changed.
> > > 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.
Gah. And I thought I was able to find a counter example in 30 seconds.
Instead, maybe I should have been getting some sleep. :)
I still don't think our project would come close to compiling if such
a warning were emitted. I would think that there are a lot more valid
reasons for ignoring a function return value than for setting a
variable value that is then not used.
-Jordan
> > > 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
