Re: [boost] Suggested coding guidelines

From: David Abrahams (david.abrahams_at_[hidden])
Date: 2001-06-29 20:58:35

----- Original Message -----
From: "Greg Chicares" <chicares_at_[hidden]>
> David Abrahams wrote:
> >
> > See http://groups.yahoo.com/group/boost/files/boost-coding-standard.html
> >
> > These are the standards I use at work. I posted these since a few people
> > expressed interest.
>
> I felt trepidation but didn't express it. I am pleasantly surprised.
> I've read every coding guideline I can find, and this is the only one
> I don't have religious issues with. It's nicely written too.

Thanks!

> > [...] In fact, I think it would be worth discussing what role they
> > should play at boost and how (whether) they should be presented.
>
> I see this as a useful expansion of
>   more/lib_guide.htm#Design and Programming
> and would recommend incorporating it there by reference, perhaps
> with a hyperlink in this bullet point:
>
>   'Follow quality programming practices. See, for example,
>    "Effective C++" 2nd Edition, and "More Effective C++", both
>    by Scott Meyers, published by Addison Wesley'
>    [begin addition] 'and these guidelines by David Abrahams and
>    Nathan Meyers' [end addition]

Noted

> | Table of Contents
> |   6.Declarations and initialization
> |   7.Comments and Documentation
>
> Suggest capitalizing only the first letter in items 7, 8, 10, 14.

Since these are section titles, shouldn't I capitalize all (important)
words, thus changing item 6?

> | 1.2 #ifndef RECTANGLE_DWA050499_HPP_
>
> Could I persuade you to use ISO8601 dates instead? I have to think
> about whether this means
>   RECTANGLE_DWA19990504_HPP_
> or
>   RECTANGLE_DWA19990405_HPP_

I'd be happy to go with ISO861, but the date is just for uniqueness; it is
not supposed to be read except by the preprocessor.

> | 1.1.   Change logs, if used, appear at the bottom of files. A source
> | file's primary role is as documentation of the current state of the
> | project.
>
> Suggest
>   documentation of the <i>current</i> state
> I personally had trouble parsing this until I realized 'current' is
> the key word.

Okay.
I note that boost conventions put change logs at the top.

> | 1.5 Of course, this technique's effectiveness is mitigated by the
> extent to which (generic) code appears in header files. [context:
> include class header first]
>
> I'm not sure I follow this. Does 'generic' mean 'templates', or just
> something like 'reusable'?

"templates", but it's parenthesized. Inline functions also mitigate it.
When there's no corresponding .cpp file, what do you do?

> | 1.7 Don't change your designs just for the sake of this guideline
> [context: avoid #include in headers when forward decl will do]
>
> I make this guideline a requirement in code reviews. I can't see a
> reason for not complying: changing the code is easy. What am I missing?
> Would s/Don't/You need not necessarily' preserve the spirit?

Sometimes doing this strictly means refactoring code to use the pimpl idiom,
lots of forwarding functions, etc. That kind of overhead is usually not
worth it. IMO.

> | 1.7 Of course, this technique's effectiveness is mitigated by the
> extent to which (generic) code appears in header file.s
>
> Same question as for 1.5 above, and change 'file.s' to 'files.'

Yikes!

> | 2.9-2.12
>
> Spacing after the section numbers is not uniform. Also occurs elsewhere,
> e.g. 4.6 vs. 4.7 .

> | 2.14  Names containing double underscores (``__'') are entirely
> forbidden.
>
> Isn't '_A' as bad as 'A__1'? IOW, why mention only one of the
> cases in 17.4.3.1.2 but not the others?

I updated it. See the latest vault submission.

> | 3.5 all uses of ``&'', ``|'', ``<<'', ``>>'', and ``^'' should be
> fully parenthesized.
>
> [Context: 'Fully parenthesize uses of bitwise operators']
> Would you allow an exception for streaming operators?
>   double x;
>   std::string s;
>   std::cout << x * x << s + s << '\n';
> Does the context imply that that's OK?

I don't follow this rule myself. Maybe we should modify it.
The relationships among & and | are not hard to keep track of. The only
trouble you have is when mixing the bitwise operators with other ones.

> | 3.6 return is not a function; do not put parentheses around the value
> being returned. This also goes for throw.
>
> Would you say the same for sizeof(unary-expression)?

No. You usually need parens for sizeof (e.g. sizeof(char*))

> | 4.2
>
> Bullet points end variously with '.' ',' and no punctuation.
>
> | 4.4 and 4.7
>
> These are very similar points and might be combined.
> [For brevity, I don't quote them here.]
>
> | 7.15 Comment out code using `` #if 0&hellip;#endif'', or ``//'', not
> ``/*&hellip;*/''comment notation.
>
> [My browser doesn't handle &ndash; either, but I think your HTML is OK.]

I should change those to ..., I guess.

> Under these guidelines, are '/*...*/' comments ever OK? If so, when?

I guess not.

> | 8.7. Re-use the keywords ``public'', ``private'', and ``protected''
> liberally to separate groups of member functions.
>
> The example illustrates this:
>
>  class foobar : public foo, public foobaz
>  {
>   public: // constructors, other public interface, etc.
>      ...
>
>   public: // interface required by base class foo
>     std::auto_ptr<foo> clone() const;
>
> Perhaps a rationale could be supplied, since (to me at least) repeating
> 'public:' doesn't seem to be the most common practice.

I don't think it's common. I find it to help readability but it's almost
purely a stylistic issue. I wrote this standard for people who didn't have a
lot of C++ experience. I think this style helps people to organize their
class interfaces, but maybe it doesn't belong in a boost document.

> | 10.5. Implementation notes appear separately from public interface and
> derivation interface documentation, preferably not in the header file.
>
> Then where should this documentation appear? I'd guess implementation
> notes belong in the .?pp file that holds the implementation. For class
> templates implemented in a header, that would be the header file. Is
> that right? Or do you mean some other file than .cpp or .hpp ?

Wherever practical. For templates that might mean in the header file or in
supporting documentation. I only wrote "preferably" not in the header file.

>
> | 11.3 When std::auto_ is inappropriate
>
> Is 'std::auto_ptr' intended?

good catch.

> | 11.4. Use assert() liberally
>
> Is 'assert()' here shorthand for 'some assertion macro or template'
> including BOOST_STATIC_ASSERT or Assert in C++PL3 section 24.3.7.2?
> Or does this imply that the macro in <cassert> is to be preferred?

Right now, it means the one in <cassert>. I agree that BOOST_STATIC_ASSERT
should be recommended for compile-time assertions.

> | 11.13 the exception-specification should be generated by a standard
> macro (e.g. ALTRA_NO_THROW)
>
> Does this conflict with 2.9? ("Avoid trademarks in names (including
> ``Altra'')."

Yes. We are still vaccilating about that policy. I seem to be the only one
in the company with opinions about it, and even I am having trouble forming
a clear one. Maybe boost will clear this one up.

> | 14.2-3 ['explicit' keyword; conversion operators]
>
> These items repeat 8.2 and say it better IMO; consider combining or
> linking 8.2 with these items.

OK.

> | 14.6 `C'-style casts are safer than reinterpret_cast<>.
>
> reinterpret_cast can't cast away constness, but a C-style cast can
> compose reinterpret_cast and const_cast in one operation. In that
> instance I'd call the C-style cast less safe. In what sense would
> it be safer?

Mmm, maybe I had the rules wrong. I can't find any counterexamples with
Comeau.

> Does this guideline mean that reinterpret_cast should never be used?

Almost.

> It has the advantage of looking ugly and being easy to grep for.

I've never needed it. Have you?

-Dave