Boost Coding Guidelines review results

From: Aleksey Gurtovoy (alexy_at_[hidden])
Date: 2001-07-27 10:34:11

The formal review period for the Boost Coding Guidelines document by David
Abrahams and Nathan Myers has ended. After all the heated discussion, it
seems that we've reached the consensus that the coding guidelines should be
ACCEPTED into boost, in the form of several separate documents targeting
different needs/audiences, see the details below. Congratulations, Dave!

There've been a lot of detailed comments from many people that really helped
the group to arrive at the final decision, thanks everybody!

The main concern of many reviewers was that the guidelines in their current
form, while being mostly excellent in contents, have a mixed nature of
"requirements", "best practices" and "stylistic guidelines", that is a
typical thing for a company coding standard (which is what the proposed
guidelines are derived from), but that, as many have noted, is not exactly
appropriate for the needs of this group. To resolve this issue, the
following changes to the document (that most of the group has agreed upon)
need to be applied:

1) refactor the current guidelines into 2 or possibly 3 separate documents;
variants are "requirements" and "guidelines", or "requirements", "best
practices" and "stylistic guidelines";

2) add/improve the wording at the beginning of each document to clearly
state what needs it is targeting, and what are the implications of not
conforming to the rules;

3) make style/terminology of each document to reflect its status. 

Also, below are some important comments (in no particular order) from which
the above list of required changes is derived, and which I would like to
highlight:

+++++++++++++++++
Darin Adler [darin_at_[hidden]]

By default make everything a guideline. Convert the entire document into 
the guidelines document. Then as time allows, promote things to be 
requirements. You and Beman can decide whether formal review is required 
or not for your initial requirements documents with your best judgement 
and depending on how many requirements you end up with and whether they 
are things that were controversial in the guidelines review process.

The requirements will presumably be a small set compared with the 
extensive set of guidelines. We can always downgrade a requirement to a 
guideline after the fact, if we decide it was a mistake.

The issues are not the same as with code, where making a change after the 
library is accepted can easily result in incompatibilities for people 
upgrading to a new version of the Boost libraries.

[another post]

It's OK to have some guidelines that many people think should "probably be 
requirements". It's probably less OK to have requirements that many people 
think should be guidelines.

[...]

We should put sets of 
both requirements and guidelines that are imperfect into Boost, and then 
refine them as necessary. Again, I stress that the guidelines are not like 
code, and there's less danger in changing them after the first version is 
accepted.

Specifically, I'd suggest no more "formal review" for the guideline part 
after the refactoring into guidelines and requirements, but perhaps 
another pass of this process on the proposed requirements part.

+++++++++++++++++
Paul Baxter [paul_baxter_at_[hidden]]

I would like to see (eventually) the documents refactored to include a
Boost-specific library policy (featuring things like commenting policy,
licensing, config policy), a set of 'standard' coding practices (probably
quite short and comprising advice like use the standard library, header
issues) and a further set of supplementary guidelines.

It may even be suitable to go further and separate stylistic guideline
issues from engineering issues.

Engineering guidelines provide a method (or methods) to help ensure that
code works robustly even though there may of course be other robust methods
e.g. noncopyable vs explicit hiding of assignment and copy constructor.

IMHO stylistic issues relate to easing the readability of code through
naming, commenting, spacing etc. (Whether variable/class naming is only a
stylistic issue may be open to debate.)

I don't think refactoring is a short task and is outside the remit of this
review. I hope that the content of the actual guidelines is what is being
reviewed such that at a later date they can be incorporated within the
existing documentation rather than as a separate overlapping document. For
now the slight overlap though confusing, is manageable.

A submission to Boost should meet the requirements of the Boost-specific
policies (albeit possibly to be implemented after review and before
acceptance). It should also meet the standard practices unless a contrary
rationale is provided and accepted. It may not necessarily meet all of the
coding guidelines and stylistically it may vary.

In summary
Boost Policy - mandatory for Boost irrelevant elsewhere
Coding Standards - mandatory for all
Coding Guidelines - a guide to good engineering practice but not mandatory
Stylistic guidelines - affects the readability - optional but needs
consistency throughout a project

+++++++++++++++++
Beman Dawes [bdawes_at_[hidden]]:

Although I usually hate all uppercase text, to help meet concerns expressed 
in the formal review, I think the guidelines document should begin with an 
all uppercase imperative similar to Dave Abrahams' current lead sentence, 
such as:

THESE GUIDELINES ARE NOT INTENDED TO BE USED AS REQUIREMENTS FOR ACCEPTANCE
OF PROPOSED BOOST LIBRARIES.

I don't think another formal review is required after refactoring, but I do 
think drafts of the documents should be posted for comments, so that 
someone can object if they think a particular requirement or guideline is 
mis-categorized.

+++++++++++++++++
Iain.Hanson_at_[hidden]:

     I would prefer to see them split into two sections rather than two 
     documents. The first section would be mandatory and would cover things 
     like naming for Interface, and 2.14. Names containing double 
     underscores ...
     
     The second section could be described as 'guidance' or 'good practice' 
     but would have over prescriptive elements re-worded to be less 
     prescriptive.
     i.e.:
     
     2.10. Data members should have a ``m_'' prefix, to distinguish them 
     from an otherwise-identical member function name and to clarify their
     provenance in code which uses them. Non-member names must not have an 
     ``m_'' prefix. 
     
     should be changed to something like:
     
     2.10. Data Member Names. It is a common practice in C++ to have a 
     naming scheme for data member variables. This helps to distinguish 
     them from otherwise identical member function parameter names. Two 
     common practicies are to prefix the name with 'm_' or to suffix the 
     name with an '_'. The naming scheme chosen is almost certainly less 
     inportant than that it be used consistenly. If you chose something 
     other than a common naming scheme, it would be useful if this was 
     noted in the documentation of the library.
     
+++++++++++++++++
Vesa Karvonen [vesa.karvonen_at_[hidden]]

The style/terminology of a guidelines document should be different from the
style/terminology of a requirements document. A requirements document
describes what is required. A guidelines document describes what is
recommended (sometimes strongly). A guidelines document should not use
phrases
like "is required" or "must be".

In principle, violation of requirements should be grounds for rejection of a
library, although singular violations could be accepted by group consensus.
On
the other hand, violation of (a few) guidelines should never cause rejection
of a library, although violation of numerous guidelines could be considered
grounds for rejection by group consensus.

Both kind of documents should be packed full of proper references to
literature. A comment such as "Every C++ textbook -- I have 10 here, not
counting C books --" is not, IMO, appropriate in a requirements or
guidelines
document, because the style is too personal.

Such documents (requirements and guidelines) should be accepted or rejected
based on whether they are *needed* by or extremely useful to library
*authors*
or not. Library users and the C++ community are important, but Boost should
not even try to impose any requirements or even guidelines for anything but
Boost library authors.

Personally, I think that there is need for a more detailed requirements
document, with examples, than the current "Library Requirements and
Guidelines" page (http://www.boost.org/more/lib_guide.htm). A guidelines
document could also be useful for library authors, but not nearly as
important
as a requirements document.

+++++++++++++++++
Mark Rodgers [mark.rodgers_at_[hidden]]

What we do need is better documentation of what the requirements for
acceptance are.

What also might be a appropriate is a "best practices" document
(completely separate from the requirements) that advises on what you
should consider putting into your own coding standards.  But that would
avoid religious issues by just highlighting the things you should consider.
Someone gave a very good example of the sort of thing it should contain
with regard to naming of member variables - basically "you should probably
distinguish them and the styles that are popular are ... "

[another post]

IMHO anything that affects the external interface could be a requirement.
Thus we really do care about such things as

- How files are named and what directories they go in.
- How classes are named.
- How public and protected members are named.
- What namespaces are used.
- How macros are named.
- Use (or not) of exception specs

What we don't care about much is implementation details because these
really are a QOI issue.  Here we can provide guidelines which are
suggestions how people can improve the (perceived) quality, but they
should be no more than that.  Thus guidelines can include such things
as

- Organisation of files (e.g. #include order, log location, etc)
- Naming of private members
- Expression spacing and bracketing
- Spacing of definitions and declarations
- Declarations and initialisation
- Comments and documentation
- Class Organisation

The rest of the stuff seems to be more advice on design, and could go in
yet another document.

+++++++++++++++++
Ronald Garcia [garcia_at_[hidden]]

IMHO, it is appropriate for a boost coding guideline to make some decisions 
that are not distinctly "best."  While some aspects of coding style
do not appear to show a "best" practice, consistency of style
can contribute immensely to the ability to read and understand code.
Picking SOME solution where several appear equally good provides a
guarantee of consistency.  

When first adopting a new style, some aspects may seem awkward to write
and awkward to read simply because old habits take time to change.
But having changed my style countless times, I find that it doesn't
take long before the new format becomes habit and transparent to read.
Being able to carry that transparency into many library reviews,
bug fixes, etc. for boost libraries would be beneficial.

I suspect that in an environment like boost, having to learn a new program
layout style every time one wishes to examine a library detracts from
the benefits that this group offers.  If one is reviewing a
proposed library, contributing new features, or fixing bugs, seeing a
consistent style can only help.  

At the same time, I think it's important that these guidelines remain
optional. I don't think that boost would reap much benefit from
requiring accepted libraries to be rewritten for style purposes.

Remember that this is an "open source" group.  I find it unreasonable
to assume that in the long run only one person will be looking at,
adding to, or modifying any given library.  Keeping the barrier of
entry low can improve the quality and quantity of collaboration.
Consistent style can improve accessibility, but it should not lead to
us rejecting quality libraries for style purposes.

+++++++++++++++++
Tim Butler [tim_at_[hidden]]

It is appropriate for a coding guidelines document to make 
some arbitrary decisions to define a 'default' style.
 
At least within modules, the style should be consistent, with
the original style taking precedence.
A file should look like it was written by one person. 
Is that a principle worth mentioning? 

+++++++++++++++++
Vladimir Prus [ghost_at_[hidden]]

The single most important thing is to refactor guidelines into boost 
conventions, semantical guidelines and syntactical ones. I would even
suggest 
specifically separating semantical guidelines which affect external
interface 
(and which are more important). Such refactoring will immediately make 
documents status clear:
        acceptance of boost conventions, will mean: "this arbitrary rules
has been 	
agreed upon for the sake of uniformity"
        acceptance of semantical guidelines will mean: "this guidelines are 
considered reasonable by boost members, who find that doing otherwise often 
causes problems"
        acceptance of syntactical guidelines, if formal review is needed for
them at 
all, will mean: "we find the suggested coding style readable and won't be 
very much embarassed if anybody use it"
Without refactoring, those different meaning will collide, and there's
danger 
readers will assume second sense, which is undesirable.

+++++++++++++++++

Once more, thanks to all reviewers who contributed their comments!

Aleksey