Coding Styleguide Updated
I have updated the Coding Styleguide. Not much has changed, content wise, but the whole thing should be a bit clearer now. Thanks Alex for your remarks.
I recently stumbled across the Joint Strike Fighter Air Vehicle C++ Coding Standards, which have recently been declassified and made available to the public. Might be an interesting read, although it has a whopping 141 pages (including one that has been intentionally left blank).
I’m not sure I like Rule 66 (or 69).
Disallowing ‘fall through’ or a jump between cases can result in duplicated code, or force the creation of pointless state objects to contain the common code in a method.
This can affect high-performance state machines.
I’d suggest that these should allow for a relaxation which *must* be documented as:
case A:
…
// fall through to …
case B:
and
case C:
…
goto case_E;
…
case E:
case_E: // re-enter for common code from C, …
…
I’m also not a fan of:
A::A():
foo(),
bar()
{
}
and prefer:
A::A()
: foo()
, bar()
{
}
But that’s aesthetics I guess. I’m more concerned at the discouragement of internal commenting. I think that’s Very Bad.
Also, are you sure that only global labels should not start with leading ‘_’? I changed to a trailing ‘_’ which can remove any issues here without seeming to uglify things.
I think Recommendation 59 is poor too. You have the entirely useless macros to define namespaces in your headers – and then if we use this in clients we’ll have a lot to change should you ever do The Right Thing and rename from e.g. Foundation to AppInf::PoCo::Foundation.
Recommendation 60 is plain silly, but that’s already been commented on in the Forum.
I think Recommendation 63 is mistaken – the *empty* exception specification is a useful hint to the compiler *and* the user. I don’t think there’s a good reason to have anything other than ‘unspecified’ and ‘does not throw’ though.
Rule 79: the number of bits in a byte????
Comment by James Mansion on August 1, 2006, 16:19
While I would personally allow for ‘fall through’ case exception where appropriate, goto is bad and shoud not be used.
—
There is nothing bad about discouraging internal comments because it encourages readable coding.
—
Leading/trailing underscores have been discussed on the forum, check it out.
—
As far as Appinf::PoCo proposal, check out http://www.accu-usa.org/Slides/ACriticalViewOfCppPractices.pdf , in Stable Practices:
[start quote]
Things To Avoid:
…
– Names incorporating company and product names.
[end quote]
I happen to agree with Kevlin on this.
—
Recommendation 59 is mighty fine as well as Recommendation 60.
—
About exception specifications, check out
http://www.gotw.ca/gotw/082.htm
and
http://www.boost.org/more/lib_guide.htm#Exception-specification
The bottom line is pretty much “don’t bother”.
—
Number of bits in a byte:
http://en.wikipedia.org/wiki/Byte
Also, from http://home.att.net/~jackklein/c/inttypes.html
Q: If sizeof(char) == 1, doesn’t that mean that CHAR_BIT is always 8?
A: The value of CHAR_BIT is required to be at least 8. Almost all modern computers today use 8 bit bytes (technically called octets, but there are still some in production and use with other sizes, such as 9 bits. Also some processors (especially D igital Signal Processors) cannot efficiently access memory in smaller pieces than the processor’s word size. There is at least one DSP I have worked with where CHAR_BIT is 32. The char types, short, int and long are all 32 bits.
Check also
http://www.thescripts.com/forum/post838797-10.html
Comment by alex on August 2, 2006, 01:43
Ok, let’s go one by one:
1) ‘fall through’ case may be fine, but goto is a no-no. No offense, but IMHO, the supplied switch/case flow control looks like an example of how not to program.
2) Underscores in names have been discussed in forums. They’ll stay _underhanded for now.
3) Recommendations 59 and 60 are mighty fine. Check out
http://www.accu-usa.org/Slides/ACriticalViewOfCppPractices.pdf
in Stable Practices/Things to Avoid items, listed among other things:
Names incorporating company and product names
4) Recommendation 63 is mighty fine too. Check
http://www.boost.org/more/lib_guide.htm#Exception-specification
The bottom line is pretty much “don’t bother”.
5) Number of bits in byte:
check following links:
http://en.wikipedia.org/wiki/Byte
http://home.att.net/~jackklein/c/inttypes.html
http://www.thescripts.com/forum/post838797-10.html
Comment by alex on August 2, 2006, 01:55
I’m sorry for double post about the same thing, WordPress was giving me hard times and would not show them.
Comment by alex on August 2, 2006, 12:31
Sorry for the inconvinience. Your comments showed up in my moderation queue. There’s a feature in WordPress that requires comments containing a certain number of links to be approved by the site admin. I have relaxed this a bit now.
Comment by guenter on August 2, 2006, 12:52
Well, I disgree about the goto thing in switches, but in a specific case as I say. The example code I have in mind is a scanner which is templated using a traits class to control the language recognised and some parts of the implementation technique. Normally I’d use re2c or ragel to generate it but in this case there was a reason not to. Suggesting that the goto case is *always* bad is foolish. Usually it is, yes. Always, no. Try writing a high-performance scanner. BTW you can’t reasonably allow fall-through and not controlled goto. If it is possible to reuse common code with a commented ‘fallthrough’ (with one specialised preamble) then why not with alternate preambles?
Are you really suggesting that simple ‘XML’ or ‘Foundation’ are good names for namespaces in a library? Its rather likely to clash, don’t you think? How do you propose to deal with it? I don’t think Kevlin produces much evidence, or at least does not suggest how to avoid clashes. I stand by what I wrote – I think using ‘XML’ is downright rude. POCO::XML is better, but if the project forks it woul then be handy to have AppInf::POCO::XML and EvilFork::POCO::XML. Users should be able to change ‘using AppInf’ to ‘using EvilFork’.
(BTW one of Kevlin’s points of ‘the root of a class hierarchy should ideally be wholly abstract’ is one I’d have some sympathy with – but POCO is pretty dismal from that point of view, and some of the changes I’m minded to suggest be would introduce some interfaces, if not go overboard. I do like my default implementations.)
I still fail to see the point of the defines for the namespaces. They don’t allow a seamless renaming, because there is no counterpart for users of the classes.
If the namespaces were likely not to clash, and the macro structure added value by allowing them to change, then there would be no problem, but I don’t think either is true.
What’s your point with the exception specification reference to boost? As it says, with a non-inline function declaring ‘throws nothing’ tells the compiler something it cannot know without whole-program optimisation which may be impossible for binary packages if not technically or practically infeasible. I agree that in the case where the function is inlined, there maybe little to be gained, but I personally would very much want to be able to change from inline to non-inline without a change of signature, and I wish to make a statement of fact that the compiler cannot otherwise deduce. I can’t see any case where adding a spceification would cause it to emit handler code that it would otherwise not have needed, since without a specification it has to assume that anything goes and local destructors may need to be run before passing an exception on.
I am aware that some systems have smallest units that are not 8 bits – but I suggest that support for them be discounted (for 9 bit systems) and that the smallest addressable unit is NOT called a byte for word addressable systems. It is likely to confuse, for little practical benefit. I’ve been there, done that, in BCPL on a mainframe if not on a DSP – but that’s showing my age.
We’ll have to disagree about internal commenting. C++ is not necessarily always clear. You gave a link to Boost, that’s a case in point. Even when code is plain, I can read what it does, but that does not tell me what the author’s *intent* was, or *why* — and such commentry is not necessarily valuable in interface documentation or at the method head, unless you want to have the guts of the algorithm repeated there in pseudo-code where it will likely rot as the code is maintained.
James
Comment by James Mansion on August 2, 2006, 16:01
I have been using Poco for about year and a half in real world projects at work and also for academic purposes, alongside other libraries, on windows as well as linux 32/64 bit platforms and did not encounter any problems with either namespace naming or underhanded names (which both btw are really easy fix, should need be, partially thanks to “Namespace_BEGIN” macros).
Namespace macros also save a level of indentation, which I find worthy. There have been requests for PoCo:: namespace before, and they may prove to be justified (in the end, it’s up to Guenter), but I won’t loose any sleep over EvilFork’s namespace naming scheme – I’ll let EvilFork worry about it.
My reasoning about Exception specification:
As boost page says, “A non-inline function is the one place a “throws nothing” exception-specification may have some benefit with some compilers.” Obviously Coding Style author thought that “some benefit with some compilers” was not worth it. Sutter also says “don’t bother”. So do I.
About “bits in byte” issue, I just wanted to point out that it is a legitimate issue. The original remark sounded like an implication that the Coding Style author was clueless.
The rest of our disagreements are mostly of “De gustibus non est disputandum” nature, so I’ll conclude the exchange with this post.
Comment by alex on August 2, 2006, 17:11
Fair enough – but I think you’re shirking the namespace issue. You think its OK for a library to pollute ‘::XML’ and ‘::Util’. I disagree. I think that’s an application writer’s business. But a library? How many libraries do you think you can use that insert names into XML and Util before you get into a mess?
Please, someone, tell me how those namespace macros can help avoid a clash in user code? Sure, they make it easy to change POCO.
And why do they save a level of indentation? That’s a bizarre thing to say. If you make the rules, then you can make one that says that you (can/should) have:
namespace AppInf { namespace POCO { namespace XML {
… // This *does not* have to be indented
} } } // close namespaces
(If that’s ugly in the implementation, well use macros. But remember that the user code doesn’t have macros and will be hardwired)
Or:
namespace AppInf_POCO_XML {
… // This *does not* have to be indented
} // close
One of the issues here is in fact: is it entirely up to Guenter?
appinf have an existing deployed customer base to protect: its not clear what the other user base is for POCO – I’m assuming its a lot less than ACE. So really there’s a question whether the library will be constrained to make life easy to support appinf’s existing code/user base (in which case some sort of forking is somewhat inevitable, if only to provide a straw man for what *could* be done) or whether the development will be ‘open’ to breaking changes, even if that means targetting a 2.0 release which is not source compatible.
I can copy it and twist it locally for my own use – but then I can’t contribute back anything else I’ve done. Its never been an issue for me until the license change.
Could I contribute an OpenSSL and GnuTLS module to the Boost-licensed public code, for example?
Comment by James Mansion on August 2, 2006, 19:18
Well, I said I’d conclude it, but allow me to just add these few general thoughts:
Ultimately, as things are right now, yes, it is up to Guenter, who btw, from my own personal experience, is very reasonable and benevolent dictator. I’m sure he’s following this discussion and will rule reasonably in the end. We all know well how impossible it is to please everyone. In any large scale project, disagreements and compromises are inevitable on all sides and fork (man, it did not take long for the ‘F’ word to surface, did it 😉 ) would, in my opinion be a pity. However, if you find it unbearable to tolerate Poco style and coding rules to such an extent that fork becomes your only option, the only thing the rest of us can say is: “may the source be with you”.
I hope you choose to stay with us, James. I myself have proposed changes that did not fly, as well as those that did. Some turned down were really not sound, after I thought a bit. Some, I still believe, were. But they did not make it in – that’s life. All well-intentioned help and advice is welcome. Some ideas may not fly, though.
Alex
Comment by alex on August 2, 2006, 21:01
My 2 cents to close this topic:
I am with Alex regarding the goto thing. The example just looks too ugly to be acceptable for me. Now, I like fast and efficient code like the next guy, but there really must be a better way 😉
I don’t think the coding styleguide totally discourages internal commenting. Internal commenting certainly has its place. There is often code that just does not seem to be sane at first glance, be it because it has to deal with some
bugs in underlying code, compiler deficiencies, or whatever. In such cases,
a clarifying comment is very welcome. However, in most cases, internal comments are just repeating what the code is already saying. These comments are plain useless and even dangerous. Someone might change the code, but not update the comment, leaving all in an inconsistent state. In my opinion, the time needed to write internal comments is better spent making the code more readable.
And finally, regarding the namespaces: I am actually thinking about getting rid of the namespace macros. The open question is whether we should introduce a top level namespace, or not. But we should discuss this in a separate post.
Comment by guenter on August 5, 2006, 16:37