Of all the gin joints in all the towns in all the world, Jordan Justen had to
walk into mine at 11:40:01 on Friday 10 July 2015 and say:
> > 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.
Well, at the time, so was I. :)
> 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.
Feedback on the _content_ of contributions is perfectly fine.
The "secret handshake" refers to the contribution _form_. If adherence to a
particular submission format (i.e. subject line contents, markups) is that
important, consider having a script to generate them, so that at least first-
timers (or infrequent contributors who might miss rule changes) will have a
better chance of getting it right. (I've seen people say: "I use a script to
process commits so if you don't format them exactly right, they may be
ignored.")
> > > > > 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 let it slide, because in this specific case it really doesn't matter.
> I would think it is more constructive to try to open a discussion on
> having the coding standards changed.
I think the odds of that happening are basically slim and none. And slim left
town.
Incidentally, looking at the document (EDK II C Coding Standards Specification
Revison 2.0), it looks like the part you quoted is section 6.4.2, not 5.4.2.
Also, section 5.1.4.2 (General Rules) shows some examples of octal escape
sequences commented using... C-style comments. :)
> > > 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.
The EDK II project code is way better than most, owing to the fact that it
supports more than just one C compiler. It's a great way to keep people on
their toes (especially people who have succumbed to the scourge of GCC
extensions).
I'm not sure how many cases of unchecked return values there would actually be
in the EDK II source (like you I thought QemuFwCfgSelectItem() and
QemuFwCfgReadBytes() were two until I looked closer), but I suspect there are
fewer than you might find elsewhere. It would be interesting to know if anyone
has run the EDK II code through any static analysis tools.
You might also try doing GCC builds with -ansi -pedantic -std=c99. This can
sometimes uncover expected things, for example:
In file included from /home/wpaul/edk/edk2/OvmfPkg/XenBusDxe/XenBusDxe.h:48:0,
from /home/wpaul/edk/edk2/OvmfPkg/XenBusDxe/XenBusDxe.c:29:
/home/wpaul/edk/edk2/OvmfPkg/Include/Protocol/XenBus.h:35:14: error: ISO C
forbids forward references to 'enum' types [-Werror=pedantic]
typedef enum xenbus_state XenBusState;
Maybe Protocol/XenBus.h needs #include <IndustryStandard/Xen/io/xenbus.h>?
Anyway, as I said before, my primary rationale for doing the change the way I
did was that when fixing something I prefer doing it with the smallest code
change possible, and in that case that was just sticking in a (VOID) cast. Had
I chosen to remove the variable entirely, I probably also would have chosen to
add the (VOID) cast to QemuFwCfgRead16(), but only because the Wind River
coding conventions have caused me to become accustomed to doing it.
-Bill
--
=============================================================================
-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
