Re: [boost] Review: Boost Coding Guidelines

From: Greg Chicares (chicares_at_[hidden])
Date: 2001-07-17 23:10:47

These are the best coding guidelines I have read. They codify best
practices in remarkably little space. The writing style is admirable.

These guidelines are useful for promoting regularity across boost,
while not imposing uniformity. They are even more useful outside
boost, for instance in shops with less experienced developers, which
might otherwise adopt standards that focus on legislating minutiae
while missing much that is truly important.

I have tried applying these guidelines to a fairly small component
I'm writing. This forced me to reconsider some decisions carefully
and resulted in useful improvements. Difficulties I encountered are
explained below.

I have spent about twenty hours studying this work. I am somewhat
knowledgeable in the problem domain, but less so than others here.

I think this document should be accepted into boost.

I offer the following comments, suggestions, and questions.

Shouldn't the copyright statement give permission for use, and
preferably copying as well?

1.2 (Unique include guards) Unlike the other guidelines, which codify
best practices in wide use, this guideline requires an uncommon
practice. If even a single guideline seems arbitrary, people may
dismiss the advice "resist the temptation to allow yourself
exceptions" in the second paragraph of the Introduction.

That said, the more I think about this rule, the more I agree with
what I perceive as its spirit. I should think an include guard like
  GNU_C_LIBRARY_VERSION_2_1_3_MATH_H
is sufficiently unambiguous that the glibc maintainers shouldn't be
faulted if they chose to use it. If that would be acceptable to the
authors, I suggest adding "For instance, " before "You may use a GUID"
and also giving examples of poor choices with an explanation of the
harm they might do--such as:

  functors_hpp is an example of a particularly poor include guard.
  It is not at all unlikely that someone else will have used an
  obvious name like this in code you want to merge with your own.
  If <functors.hpp> is included only via other headers, tracking
  the problem down might take a long time--and compiler diagnostics
  won't be much help. Similarly, avoid names like tools_hpp or
  my_typedefs_hpp.

For me, the 'cout' vs. 'std::cout' debate was settled when everyone
agreed you mustn't use 'cout' in headers, and someone added that a
different style for .cpp files is inconsistent--and some anecdotes
illustrated actual harm in the real world from omitting 'std::'.
I'd like to see such argument-settling rationale or examples here.

1.4 (Lower-case filenames) I would recommend that names with
embedded spaces also be forbidden. Suggested addition:

  Similarly, file names must not contain embedded spaces,
  because that would cause problems on many platforms.

Some time ago there was a discussion here on a maximum length for
filenames. Should some maximum be given? To ask that another way:
Is a 300-character filename ever good? The longest file name in
boost-1.23.0 is

  123456789012345678901234567890123
  simple_segregated_storage_fwd.hpp

so I propose for discussion

  File names are preferably limited to fourteen characters,
  so that portability to any POSIX system is guaranteed.
  File names longer than forty characters are never needed.
  
1.5 I see "To be sure" and "to help ensure" as conflicting.
Suggested alternative wording following the first sentence:

  You can be sure it's safe to #include a header anywhere only if
  some source file #includes it before anything else. Otherwise, a
  dependency on the order of #included files might be overlooked.
  For a header that doesn't naturally correspond to any source file,
  you can ensure safety by adding a source file that does nothing
  but #include it.

2.9 (trademarks) "Altra" need not be mentioned in boost standards.
Therefore, suggest removing last sentence of second paragraph in
following Discussion. Do you still want "ALTRA" in 11.13?

2.13 "Preprocessor macros should be avoided. Where neccessary, macros
are ALL_CAPS." Second sentence could be read as "where capitalization
is necessary..."; suggest "When they are unavoidable" instead.

3.5 "Fully parenthesize" has a hyphen between the words in one place
but not the other. Consistency is more important than which is chosen.

Discussion following 3

  "even GNU doesn't suggest a space before other unary operators..."
  "Only GNU departs from this industry-wide standard..."
and in 5.2:
  "The GNU indentation style introduces visual clutter..."

There's no need to criticize another set of guidelines. GNU partisans
might be turned off by this.

4.8. "Access specifiers...are indented one space..."

Why would two spaces be less good? Sections 4.6 and 5.8 allow some
choice in the number of spaces; why not this section too?

Example following 5

  "set_timeout_time" versus "timeo_time" and "timeo"

I suppose the last two are "timeouts" too. If so, why abbreviate,
given that 2.5 says "Iterator" is better than "Iter"?

6.1. "Only one name may be declared in a definition statement."

Why allow more than one in a declaration statement?
  int a, *b, &c, d;
is almost as bad as
  int a, *b = -1, &c(3), d;
OK, that violates guideline 6.2; but it seems simpler to draw the line
so that this
  int a, b, c, d;
is also not allowed.

6.2 "This guideline and the previous one are corrolaries"

Perhaps "correlated" is intended. A corollary follows immediately
from a primary proposition, but neither of these is primary--they're
interrelated.

6.7 "Place const after the base type it modifies. [This is] consistent
with compiler-generated type descriptions in error messages."

Consider

  int main()
  {
      const int a = 3;
      int const*const b = a;
      int const* c;
  }

borland C++ 5.5.1:
  Cannot convert 'const int' to 'const int *'
g++-2.95.2-1:
  initialization to `const int *const' from `const int'
  warning: unused variable `const int * c'
  warning: unused variable `const int *const b'
so some compilers put 'const' before 'int' in diagnostics.

The accompanying example says:

  char* const buffer = &s[0];      // constant pointer to mutable char
  char const* log(const char* s);  // mutable pointer to constant char

Recommend changing 'mutable' to 'modifiable' to avoid confusion with
the 'mutable' keyword.

7.6. "Comment every virtual function only at its least-derived point
of declaration."

Suggest "Comment the interface of every...". There must be a right
place to comment additional or variant behavior provided by a derived
class overload. Wouldn't that be in the (derived) implementation?

7.9 I'm slightly puzzled by the interplay between 7.3
  "Accuracy is more important than brevity."
and 7.9
  "Strive for brevity inside of class declarations."
I read it as "accuracy above all, but edit carefully for brevity too".
If no amount of editing can achieve this, then it seems best to move
at least part of the documentation outside the class, perhaps into
a separate HTML file. I think random::inversive_congruential got this
exactly right. Suggest adding at end of 7.9

  Some classes require lengthy documentation; this should appear
  immediately before the class declaration or in a separate HTML file.

7.11 (Comments are correct English sentences) should apply to the
examples--for instance, following section 8:
  // nested class declaration
  // nested class definition outside enclosing class
  // helper functions
Or perhaps the guideline should allow noun phrases like these.

8.1 This appears to forbid any non-public constructor or destructor,
but there are idiomatic uses for such things. It also appears to
forbid private typedefs; why aren't those OK for a type used only
in the private interface?

8.4 "...Virtual destructor bodies should usually not be inlined,
so use caution when defining these inline."

I think this guideline can be improved by deleting its second clause.

The Discussion should give a rationale. Is it the same as Lakos's
recommendation (9.3.3 of his book), which is based on the idea that
compilers will follow CFront in placing the vtable in the module that
defines the first non-inline virtual function? If so, recommend also
requiring that the dtor be the first virtual declared.

Or is it that a compiler might internally use a pointer to the dtor,
so that it will generate a copy out of line anyway? In that case,
shouldn't the same advice apply to inline constructors?

8.6 (Repeat access-specifiers liberally) I think this diverges from
common practice, and therefore requires a strong rationale.

This gives up a guarantee that the standard [9.2/12] would otherwise
provide. I don't know what problem that guarantee was intended to
solve, but this guideline unsolves it.

9.4 Suggest changing 'subclass' to 'derived class' here and in the
example following section 8. D&E 2.9: "I never could remember what was
'sub' and what was 'super' and observed that I was not the only one
with this particular problem."

  "Provide a non-empty, non-pure virtual function only when you expect
  a subclass to completely replace the function's implementation."

Please help me understand this. If I want to require a derived class
to provide an implementation, don't I declare the function pure in
the base class? If I make a virtual non-pure and non-empty, doesn't
that mean that some derived classes may use the base implementation?

10.1. "A component is a class or small set of closely related classes
and functions, usually in a single header file."

Suggest "usually declared in a single header file".

10.4 "Concepts (e.g. forward iterator, assignable)"

See 2.3--shouldn't these be written ForwardIterator, Assignable?

Examples following 10

  // Invariants:
  //   1. this->container is either 0 or points to a valid container
  //   2. if (this->container != 0) this->container->size() > 0.

Are 1 and 2 different?

If so, then recommend adding an assertion implementing each.

If not, then I'd read 2 as the assertion body corresponding to the
English sentence 1. In that case:
 - recommend changing 2 to
     assert(this->container != 0 || this->container->size() > 0)
 - recommend changing "valid" to "nonempty" in 1

11.16 (reference to _Writing Solid Code_)

IMO it's regrettable that the only book cited in the guidelines is
this one--that could be taken as a ringing endorsement. It's a useful
book if you know which of its suggestions should be ignored. But see
Sean Corfield's "Not Recommended" review at accu.org.

There are better books. Nothing is lost if this reference is removed.

12.2 "Each module should be in a separate sub-namespace."

"Component" is defined (10.1); would it make sense to use it here in
place of "module"? (I read "module" as "source file".)

12.6 Unnamed namespace..."expectation" that names won't collide.

Why not say "assurance" instead of "expectation"?

I think it would be beneficial to say explicitly that an unnamed
namespace is preferable to 'static' at namespace scope (which [D.2]
deprecates for objects, but not for functions).

15.5 "Beware reference data members"

Shouldn't the same advice apply to const members?

Typos

1.2 s/neeeds/needs/

2.13, 6.2 s/neccessary/necessary/

6.2 s/corrolaries/corollaries/

examples following 6.7 s/neccessarily/necessarily/

examples following 8.8 s/retuned/returned/

discussion following 8.8 s/neccessary/necessary/
  and /covenient/convenient/

discussion following 9.7 s/accessiblity/accessibility/

10.2 s/suppports/supports/

11.10 s/tempation/temptation/