Re: [boost] Suggested coding guidelines

From: David Abrahams (david.abrahams_at_[hidden])
Date: 2001-06-30 22:23:27

----- Original Message -----
From: "Greg Chicares" <chicares_at_[hidden]>

> > Since these are section titles, shouldn't I capitalize all (important)
> > words, thus changing item 6?
>
> I think any consistent style is fine.
> If 6 changes, then maybe 1, 3, 4, 5, 9, and 11 should too.

OK. This is really nitpicky!

> > > | 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.
>
> A feature isn't truly cool until someone uses it for some purpose the
> inventor didn't anticipate. This meets that criterion in a way that
> you might not intend. As long as a date is there, people will
>   read it,
>   grep/sed/awk for it,
>   believe/trust/depend on it,
> and, since this is a coding guideline, ask questions like "do you
> update it when you change the file?"

Answer is no, but point taken.

> Have you ever run into a problem using just something like
>   RECTANGLE_HPP
> anyway?

No, but it's easy to imagine as software gets larger and different libraries
are used together.

> In place of
> | Incorporate your initials and the date as shown in this example.
> I'd suggest something like
>   Form the include guard name from the filename, replacing periods
>   with underscores. For additional safety, you might wish to add
>   your initials and the date as shown in the example.

I am not willing to change it from a mandate to a suggestion. I am willing
to add an explanatory note and a note saying that you shouldn't rely on
#include guards being written any particular way.

> > > | 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.
>
> OK, implementation in header <--> less chance of unnoticed dependencies.

That's not what I meant. I meant the technique can't be used when there's no
corresponding .cpp file.

> > When there's no corresponding .cpp file, what do you do?
>
> Doesn't matter. Given a header, this rule doesn't require the existence
> of a .cpp file that must include that header first.
>
> Take lexical_cast.hpp as an example. There's no corresponding .cpp file
> for the rule to apply to. This header never gets included first
> (except in lexical_cast_test.cpp, which is a special use). It's the
> limiting case: complete mitigation of the technique's effectiveness,
> by removing the problem the technique would solve--a good thing.

It's easy to imagine ways in which lexical_cast.hpp would fail to include
something needed in order to use lexical_cast<>, and how that might go
undetected depending on how lexical_cast.hpp were included. It would make
sense to treat lexical_cast_test.cpp as the corresponding .cpp file.

> > > Would you say the same for sizeof(unary-expression)?
> >
> > No. You usually need parens for sizeof (e.g. sizeof(char*))
>
> Only for sizeof(type-id), not for sizeof(unary-expression).
> But forbidding unnecessary parentheses in the latter case
> goes beyond ordinary pedantry, so the guideline's probably
> better left as is.

Thank goodness!

> > > | 7.15 Comment out code using `` #if 0&hellip;#endif'', or ``//'', not
> > > ``/*&hellip;*/''comment notation.
> > >
> > > Under these guidelines, are '/*...*/' comments ever OK? If so, when?
> >
> > I guess not.
>
> The guideline's OK, because it doesn't literally forbid
>
>   /*
>     block comments like this
>     which may be many line long
>     and contain characters like ' (apostrophe)
>   */

I think it's supposed to. /* */ doesn't nest and thus is subject to
surprises.

> although I'd rather it said something like
>   'Use /*...*/ only for multiline text comments.'
> gcc is strict about the undefined behavior of
>
>   #ifdef 0
>     won't work with gcc
>     what preprocessor token would the lone apostrophe belong to?
>   #endif

That's undefined? Wow.

> and recommends /*...*/ instead for *text* comments; I just
> don't want to see that usage deprecated.

OK.

> > I wrote this standard for people who didn't have a
> > lot of C++ experience.
>
> Depends on your definition of "a lot"! I'd say these guidelines assume
> considerable knowledge. That's a strength--it keeps them from being
> verbose, and encourages the less knowledgeable to figure out what they
> don't know yet. I plan to use this as a learning and training tool.
>
> > > | 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.
>
> Yet see the comment on polymorphic_downcast in boost/cast.hpp
>  // WARNING: Because this cast uses assert(), it violates the One
Definition
>  // Rule if NDEBUG is inconsistently defined across translation units.
> An assertion facility that doesn't depend on NDEBUG shouldn't
> be a violation.

OK

> > > | 14.6 `C'-style casts are safer than reinterpret_cast<>. [...]
> > > 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?
>
> Pass a string via windows messaging, which wants an integer type
>   ::SendMessage(*fw, DestID, 0, reinterpret_cast<long>(s.c_str()));
> Catch the long int elsewhere and treat it as a char*
>   SomeControl->SetText(reinterpret_cast<char*>(LParam));
>
> I just grepped all the code I've written here over the past
> several years, and aside from windows stuff, I find only two
> uses of reinterpret_cast. Oops...neither of them looks right.
> The point is I could find them easily. It almost screams
> "nonportable, possible error" and is safer in the sense that
> it attracts scrutiny and is likely to be removed if bogus.
> If my two uses (windows aside) were in current production
> code, I'd reevaluate them right now.
>
> So I'd agree that reinterpret_cast should almost never be used,
> yet add that C-style casts should be used even more rarely.

Since you seem to care about this a lot, why don't you send me a proposed
patch for all of these things. I'll look over the changes and integrate
them, unless you do something that incites a religious fervor on this end
;-)

-Dave