Re: Review: Boost Coding Guidelines

From: Tim Butler (tim_at_[hidden])
Date: 2001-07-24 01:40:01

I vote yes, the coding guidelines should be accepted.

I look to Boost for professional, peer-reviewed material
and I would love to see a set of coding guidelines as part
of that. 

It is clear that these are guidelines (as opposed to requirements).
It is fine with me for a guideline's doc to use words like
'required' or 'banned' because the context is still within a 
a set of suggestions. 

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? 

I'm really not worried that the Boost community will go insane and
start rejecting libraries based on arbitrary style issues.
If we do then we deserve what we get (or don't get).

A person can vote no on a library for any reason, regardless of the 
existence of a guidelines document.

Here are some comments, ordered by their appearance in the document,
not importance. None of these issues would prevent me from 
accepting the document. 

------------------------------------------------------------------
> 1.3.   Avoid preprocessor macros (i.e. #define), use ...enums 
> or constants (for other macros) instead.

o Should this refer to the 
  Coding Guidelines for Integral Constant Expressions 
  ( http://www.boost.org/more/int_const_guidelines.htm ) ?

o How do folks like to implement c-string constants in header files? 

------------------------------------------------------------------
> 1.7.   Avoid header file dependencies. If the header file only needs to use 
> pointers or references to a given class, a forward declaration can be used  
> in lieu of including the header which contains the class' definition.

o You might also mention:
    The type of a parameter or the return type for a function declaration
    that is not a definition may be an incomplete class type.

  I remember being surprised the first time I learned that. 

------------------------------------------------------------------
> 2.9.   Avoid trademarks in names (including ``Altra''). 

o 'Altra" appears in other places too.

------------------------------------------------------------------
> 3.7.   Break expressions before an operator if possible, 
>        when they get too long. 
> ...
> Examples:
> // Right:
> operator==(char)
  
o  This should look like a complete delcaration (Right & Wrong examples).

------------------------------------------------------------------
> 4.9 ...
>   template<typename T>
>   void template_function(T* target,
>       std::basic_string<T> const& source);

o I guess 'target' should be on its own line.

> int 
> database::roll_back_transaction()
> { ... }

o I prefer this form of definition, but it violates 4.3.
  
------------------------------------------------------------------
> 6.2.   Place * and & with the type name, not the object name. ...
> we eschew the compact but unclear style 
> inherited from `C' which makes it neccessary 
> to bind type qualifiers to the name they are qualifying.

o What is the requirement in 'C' that C++ does not impose? 

------------------------------------------------------------------
> 6.7. Place const after the base type it modifies. This simple rule makes it 
> easier to write type declarations correctly and will make your declarations 
> consistent with compiler-generated type descriptions in error messages. 

o I disagree with this simply because I suspect the "const before" 
  form is more familiar to most people. I think most people find the 
  *compiler* messages confusing and I don't think it is valuable 
  making them spend extra effort interpreting declarations in the code
  with an unfamiliar style. In fact there are a couple instances 
  in this document that appear wrong but require a "double take".

  You might rephrase this:

  Place const before the base type it modifies. Every C++ textbook
  does this and so does the C++ standard. :) (please note smiley)

  Also, when I see a 
    const int = 42;
  its easier to imagine the 'const' filling a position for 'static'.

------------------------------------------------------------------
> 8.5.   Nested-class definitions should be defined outside the nesting 
> class body, wherever possible.

  That's neat!

> // Schnix is expected to blah and is called when blah.

o Lowercase 's'.

------------------------------------------------------------------
> 9.5. Avoid calling a base class' version of a virtual function explicitly, 
> especially from the derived class' override. The need to do this is usually 
> an indication of careless design in the base class. Instead, refactor the 
> base class implementation as detailed above. 

o ouch. Okay so I've been known to do some careless design :).
  But seriously, I believe there are legitimate situations were
  I have needed to call a base class virtual function. One example
  was an Init function (where I couldn't do it in the constructor
  because the object was dynamically loaded).  
 
  Would it be okay to mention the idiom of a protected virtual indicating
  that derived classes should call the base function?
  ... With extreme prejudice? 
  I'm sure I'll catch hell here :)

------------------------------------------------------------------
> 11.3. Document transfer-of-ownership. Using std::auto_ptr is the best way; 
> it  simplifies function naming and enforces correctness in code. 

o Someone unfamiliar with returning an auto_ptr from a function 
  might not understand what you are talking about. 
  You might refer to the example in section 8 or describe the 
  technique.

> When std::auto_ptr is inappropriate, name prefixes like 
> create_ and adopt_ can help. 
  
o You might describe that create_ gives up ownership and
  adopt_ aquires ownership. Or is that too obvious?
  Is orphan_ worth mentioning?

------------------------------------------------------------------
13 Overloading ...
> char* next_word(char* s);
> char* const next_word(char* const s);

o This is a dubious use of const. You probably meant

  char const* next_word(char const* s);

------------------------------------------------------------------
> 14. Type conversions ...
> class my_string
> {
>  public:
>    char* const c_string() const; // right
>    operator char const*() const; // wrong

o You probably meant

     char const* c_string() const; // right

-tim