Re: Suggested coding guidelines

From: williamkempf_at_[hidden]
Date: 2001-07-06 17:38:49

--- In boost_at_y..., "David Abrahams" <david.abrahams_at_r...> wrote:
> [Nathan, one of these could use some interpretation from you]
> > 1.6  Does this bar comments from being placed above the #includes 
and
> > #include guards?
> 
> I guess so, though I think its unintended. If you'd like to supply 
new
> wording allowing initial comments I'm not opposed.

I'm not the best documentor, but if no one else has a fix I'll give 
her a go ;).
 
> > 4.1  Should the guidelines specify the number of spaces used for 
an
> > indentation level?  If so, should it be consistent for all
> > indentations?
> 
> indentation to indicate namespace scoping can use up a lot of real-
estate.
> Since there's almost no information at the level of indentation 
where the
> namespace is declared, it isn't worth wasting space on. Also, 
indentation is
> only useful if you stand a reasonable chance of seeing the indented 
and
> unindented stuff at the same time. Namespaces tend not to have that
> property. That's why it's 2 for a namespace and 4 everywhere else.

Understandable.  My only complaint is that editors that do auto-
indenting and/or treat the tab key as a sequence of spaces may not 
work nicely with varying indent sizes.
 
> > 4.4  Why?  As long as all lines but the first in a multi-line
> > definition is indented another level the blank lines add little
> > benefit, and in fact can detract from the rest of the code.
> 
> IMO vertical density makes reading code difficult. If you stick 
several
> multi-line definitions next to one another, things tend to get too 
dense.

Granted.  However, you can arrive with a different problem if you 
expand the vertical density too much, which this rule can lead to.  
I've never used this rule, but been careful to insure the vertical 
density remains at a level consistent with easy reading.  It just 
seems like this rule is a little too restrictive just to inforce 
another rule that should be more explicitly spelled out.  In other 
words, I'd prefer to see a rule about insuring the vertical layout is 
properly split into "paragraphs" for easy consumption.  (Granted, 
wording of such a "rule" will be difficult.)

> > 4.5  See 4.1.
> 
> Consistency of indentation of continuation lines is less important 
than the
> other one. I didn't think it was worth mandating.

Well, I'm not sure the number of spaces *should* be mandated.  As 
evidenced here, different people have different preferences for 
indent size.  As long as the size is consistent within a file should 
we dictate the actual size?  Also, this bullet has the same complaint 
I detailed above concerning editors.
 
> > 4.6  Wording should indicate this applies to multi-line 
definitions.
> > As written it means that ALL function definitions should be multi-
> > line.  Also, see 4.1.
> 
> I got this guideline from Nathan. I always read it to mean that all 
function
> definitions with arguments should be multi-line. Of course I don't 
follow
> that practice myself. I like your reading better. Nathan, what do 
you have
> to say about this?

So all definitions would follow the form:

void foo(
  bar b)
{
}

I hope this wasn't the intended interpretation.
 
> > 4.7  Again, why?
> 
> Same reasons.
> 
> > 4.8  Why?  Also, see 4.1.
> 
> It looks nice ;-)
> I used to indent them zero spaces, but worked somewhere that they 
were
> always indented 1 space. I grew to like it. It shows that the class 
body
> isn't over yet. I'm not wedded to this one.

I'm opposed to this for the same reason I'm opposed to the other 
variable sized indentation rules.  I'd much prefer to stick to a 
single indentation size (whether mandated or not) and leave these
unindented.
 
> > 5.7  I find this a little restrictive with the reasoning being
> > questionable.  That said, I can live with it.
> 
> Here's an example of the alternative:
> 
> 
>     if (x < 0)
>         x = 0;
>     else
>     {
>         for (y = 0; y < x; ++y)
>             do_something();
> 
>        do_some_other_thing();
>     }
> 
> My eye says that the x=0 goes with the 'if' clause and the rest is a
> separate entity like a struct declaration or a while loop. This is 
an
> aesthetic consideration; others' taste may not coincide here.

Mine don't, but I realize this one is purely aesthetic, that's why I 
can live with the rule.  However, since you agree that it's purely 
aesthetic, and aesthetics are in the eye of the beholder, should we 
really dictate (even in "optional" guidelines) this?
 
> > 5.8  See 4.1.
> >
> > 6.7  This makes the code read less like english.
> 
> Depends whether you think const is an adjective or a noun. I think 
it's a
> noun ;-)

*laughs*  I guess.  I definately don't agree with you, but...
 
> > Also, the comment
> > about being consistent with compiler error messages is, frankly,
> > misguided.  We can't predict how compilers will display this sort 
of
> > message.
> 
> Except that compilers have to go out-of-their way to 
generate "const char"
> instead of "char const" when they also have to generate "char* 
const". If
> you pick a simple algorithm to translate an internal type 
representation
> into valid C++, it will always put the const afterwards.

Granted.  However, since the standard doesn't dictate this Murphy's 
law says there will be an implementation that behaves differently 
from what you'd expect.  I'd rather not document this at all, but if 
you're going to make it obvious in the wording that you're just 
expecting *most* compilers to generate such errors.
 
> That said, I still have a hard time adjusting to writing "char 
const". OTOH,
> if you have to pick a rule, it should be a consistent one. The 
alternative
> is no rule: some people write "char const" and others write "const 
char". Do
> we care?

Personally I don't.  If we have to pick and it's up to a vote I'll 
vote for "const char", but I'm used to adapting styles according to 
guidelines I don't always agree with. :)

One should note, however, that the C++ standards document uses 
the "const char" form in most (all?) examples.  One can make an 
argument for maintaining consisty here.

> > 7.3 & 7.5  Don't quite work well together.  Combining them will
> > better explain that public and protected interfaces should be 
fully
> > commented at the point of declaration.
> 
> Please suggest new wording.

Well, again, I'm not great at writing documents.  Having the rules 
seperated and worded as they are leads to some confusion, but I'm not 
sure how to rework this.  Maybe I'll tackle this and the previous one 
later and send my attempts to you.
 
> > 7.3 & 7.9  Though they technically don't conflict, they lead to
> > confusion because of their differing emphasis on accuracy or 
brevity.
> 
> Yes, they are explicitly recognized as conflicting goals. Do you 
have a
> suggestion?

Other than putting text in that states what you just did, no.  I 
understand there's a trade off here and the programmer has to try and 
achieve as much of both goals as he can.  The problem is, both rules 
currently read as mandates rather than goals, and given the conflict 
you can only fully meet one mandate.  I'd suggest lessening the 
emphasis or combining the two into a single rule stating that you 
must strive for terseness while being complete.
 
> > 7.12  It would be helpful to have a document listing the proper 
C++
> > terminology and improper variants.
> 
> Would you like to write one?

*laughs*  No, because I'm often caught using the improper variants.  
That's precisely why I'd like a document listing the proper ones.  I 
expect such a document would be a living document, added to 
frequently (at least at first).  I'm just not qualified to start it, 
sorry.  Guess I was fishing for someone else to volunteer.
 
> > 7.15  Commenting out code should be avoided.  Yes, there are times
> > when this rule can be broken, but generally it's better to rely on
> > the configuration management tool (CVS for us) for recalling code
> > that's been removed.  Commented out code simply makes reading more
> > difficult.
> 
> Changed to:
> 
> Comment out code using `` #if 0...#endif'', or ``//'', not 
``/*...*/''
> comment notation. Avoid checking in code that's been commented out. 
Since it
> doesn't get tested, it will likely not make any sense tomorrow even 
if it
> makes some sense today, .

If the code never gets checked in we don't need guidelines for how to 
comment out the code. :)  Other than that, I like this wording better.

> > 8.1 & 8.2  Though use of public and protected data members may be
> > questionable from a design stand point, this is a coding 
guidelines
> > document not a design guidelines document. (Boost should have 
both,
> > but the coding guidelines should be careful to not discuss any 
design
> > guidelines.)
> 
> Even if I agreed that it would be a good idea to separate the 
documents (I'm
> not sure I do), I am definitely unwilling to invest the effort 
required. If
> you would like to generate a consensus for two documents and do the 
editing,
> though...

Reasonable response.  I'll wait to see if others agree and if someone 
else is willing to do the effort.
 
> > 8.4  Why make an exception for operator()?
> 
> It's not an exception for operator(), it's for simple function 
objects.
> These are just simple functions in disguise; separating interface 
from
> implementation is more trouble than it's worth in my experience.
> 
> > The reasoning given just
> > begs the question since the word "often" is used.
> 
> Where is the word "often" used???

"Function objects are _often_ just wrappers over a simple function or 
function template." (emphasis added by me).

> > What about the other cases?
> 
> I think the wording is clear: there are no "other cases".
> 
> > What about in template classes?
> 
> "Function definitions in the class body are forbidden"

I can't always apply this rule with VC++ :(.  It often gets confused 
when the definition of template members are defined outside of the 
class.  Also, though I agree with the reasoning, templates are rarely 
coded this way, and there's something to be said for consistency as 
well.  I won't argue this rule too much, but had to voice some 
concern.
 
> > 8.7  Why?  The grouping and commenting I can agree with, but 
leaving
> > off the virtual can lead to confusion for readers not fully 
versed in
> > C++.
> 
> The rationale is in the discussion section below:
> 
> "Writing `virtual' on the declaration of a virtual function 
override is
> neither neccessary nor sufficient to document where it comes from 
and which
> code it interacts with. Leaving the keyword off in derived classes 
reduces
> clutter and thwarts the temptation to treat it as sufficient 
documentation"

I think it adds little clutter, and the grouping rule enforces coding 
that doesn't treat the keyword as enough.  Since the keyword does 
make the code easier to comprehend for some users I just don't think 
that requiring it to be left out is the right choice here.  Again, 
though, I've voiced my opinion once, I'll live with what you decide 
after hearing my arguments :).
 
> > 12.2  What does this mean for Boost where the convention is to put
> > all code in the boost namespace?
> 
> I think the emergent rule for boost is: separate domains go in 
their own
> sub-namespaces. General-purpose code goes in boost. Probably 12.2 
should be
> changed to reflect that fact.

I realize it's "emerging", but is that really the current desire?  
I'm asking because I'd like to know if the Boost.Threads stuff should 
be in boost::thread before I submit (would this cause any conflicts 
with the thread class name?).

Bill Kempf