Re: [Fresco-devel] Coding guidelines again

On Thu, Jun 13, 2002 at 09:16:18AM -0400, Stefan Seefeld wrote:
> 
> Nathaniel Smith wrote:
> 
> > - Return type and function name on separate lines
> >     void
> >     foo(int bar)
> >    rather than       
> >     void foo(int bar)
> >
> >   Justification: Return types in C++ can be pretty long; this reduces
> >     wrapping and is easier to read to boot.
> 
> can't we make this optional ? 'void foo(int bar)' should really be
> a one-liner. I agree that wrapping can help if the whole gets too long
> so we have to wrap either the argument list or the return value or both.
> But if it fits into a single line, I don't think we should mandate wrapping.

I'm not sure.  It's only partly about return types being long; I've
been trying this style in a bit of code, and actually finding it
more readable.  It puts each thing I want to look at (return type,
function name, arguments) in places that are totally distinctive
visually (beginning of line, or inside parens).  K&R style braces also
save vertical whitespace, but aren't as visually distinctive...
Have you tried it this way?

> >    If you have an initializer list, put the ':' after the closing ')'
> >    of the parameter section and start a new line.  If the
> >    initializers all fit on a single line, put them on a single line;
> >    otherwise put each on a separate line.  [[FIXME: rewrite this
> >    rule, and break it into multiple sub-rules]]
> >    So:
> >     class Foo {
> 
> uhm, shouldn't that read
> 
> class Foo
> {
> 
> according to the proposed style ? At least that's what I'm using and it 
> is consistent with the bracing styles mentioned elsewhere...

Yes, it should.  I've been bad about systematically going through and
fixing the examples; whenever the examples are inconsistent with the
rules, it's just because I missed fixing that example...

> >         // Correct:
> >         Foo() :
> >             my_bar(0), my_baz(0)
> >             {};
> 
> no semicolon please !

*embarrassed blush* Actually, I just stuck semicolons on everything,
because I can never remember which things require it.  It's gone now
:-) 

> >         Foo(int super_special_long_named_variable) :
> >             my_bar(0),
> >             my_baz(new baz_type(super_special_long_named_variable))
> >             {};
> what is wrong with
> 
>            Foo() : my_bar(0), my_baz(0) {}

Hmm, yeah, that looks nice.

>            Foo(int super_special_ling_named_variable)
>              : my_bar(0),
>                my_baz(new baz_type....)
>            {}
> 
> note the braces being on the same level as 'Foo', while the initializer
> list is indented.

Hrm.  Part of me dislikes the idea of putting anything in that first
column besides types and member names, but perhaps that part of me is
being silly.  I'm also a little confused about indenting the colon 2
spaces... that seems inconsistent, and could make commenting lines
out a little wierd (I guess you'd do something like ": // my_bar(0),").

> >         int
> >         bar() { return my_bar; };
> >         void
> >         baz_incr()
> >             { if (my_baz) my_baz->references++; };
> >         void
> >         quux() {};
> >         inline void
> >         blargle();
> >         // Incorrect:
> >         void
> >         baz_incr2()
> >         {
> >             if (my_baz) my_baz->references += (::should_do_it ? 1 : 0);
> >         }
> 
> I find this significantly harder to read than one function declaration 
> per line (i.e. no wrapping of return types).

Did you read my response to Mourad?  Do you think it'll still be that
hard when you have syntax highlighting and synopsis comments between
each declaration?  Hard enough to justify inconsistency?  I know what
you mean, but think there's a net win for real code.

> >         ...
> >     };
> >    Anything longer should go after the class declaration:
> >     inline void
> >     Foo::blargle() {
> >         ...
> >     }
> 
> again, what about:
> 
> inline void Foo::blargle()
> {
> ...
> }
> 
> i.e. (aside the return type not being wrapped) braces consistently
> wrapped with other blocks.

'nother stupid mistake on my part.

> >
> >   Justification: Don't clutter up declarations with code, unless the
> >    functions are so trivial that it doesn't matter.
> 
> right
> 
> >
> > - Inside a class declaration declare things in the order:
> >     1. public
> >     2. protected
> >     3. private
> >     4. operator
> 
> uh, why are operators special ? Are you speaking of 'friend' operators ?
> Then they should be part of the public section for obvious reasons.

I have no idea, I just grabbed that from what someone else posted :-)
Here's a new version:

 - Inside a class declaration declare things in the order:
     1. public
     2. protected
     3. private
    Conceptually, friend operators are probably public, and internal
    friend classes are probably private.  Use your judgement.

Is that better?

> Other issue: each section (public, protected, private) should start with
> type declarations (if any), followed by methods, (static, then 
> ordinary), followed by member variables (static, then ordinary).

Added new rule to this effect.

Hmm, any particular reason for that order?  Eg, it might make sense to
group things by static-ness first, and only then method/variable
status.  Or not, I don't know.

> >   Justification: The more likely people are to need something, the
> >    closer it should be to the top of the list.
> >
> > - Class access labels (private, public, etc.) should be indented 2
> >    spaces relative to the start of the class.  Eg:
> >     class Foo {
> 
> class Foo
> {

Yes.

> >       public:
> >         Foo();
> >         ~Foo();
> >       private:
> >         ...
> >     };
> >
> >   Justification: Members are logically contained by the class
> >    declaration, so should be indented 4 spaces relative to that.
> >    Access labels should be indented intermediate between the class
> >    and the members.  Hence, 2 spaces.
> >
> >
> >Naming
> >------
> > - Naming:
> >    Name classes and namespace portions LikeThis (StudlyCaps, with the
> >     first letter capitalized). Eg:
> >      class FooBar {};
> >      namespace FooBar::BazQuux {};
> 
> no semicolon after namespace definition ! (The semicolon after the class 
> definition is needed since C++ allows a class definition to be written 
> with a variable declaration in the same declaration, i.e.
> 
> class Foo {} foo;
> 
> so the semicolon is needed to disambiguate. Not so with namespaces.

Fixed.

> >    Name everything else like_this, with underscores separating words.
> >     Eg:
> >      int FooBar::baz_quux() {};
> >      static int foo_bar;
> >    Constant variables and macros should be capitalized, LIKE_THIS.
> 
> macros should be entirely banned.

I say this later... I'll add a parenthetical comment to this effect
here.

    Constant variables and macros (which you will never use) should be
    capitalized, LIKE_THIS.  This includes enum values. Eg:

Does anyone have a problem parsing that?  I'm worried that it might
read to some people as saying you shouldn't use constant variables.

> >     This includes enum values. Eg:
> >      static const int FOO_BAR = 1;

Incidentally, I just noticed that we rarely, if ever, capitalize our
enum values -- see Berlin/include/Berlin/PNG.hh, or
Berlin/include/Berlin/PositionalFocus.hh, or
Berlin/include/Berlin/DebugGraphic.hh, or ...

I haven't worked with these enum's very much -- maybe someone else can
tell me whether capitalizing them is actually a good idea?

> >    Private variables should have "my_" prepended to them.  Private
> >     methods should have "_" prepended to them.
> >
> >   Justification: It's much more readable than theStupidJavaWay, etc.
> >    See http://www.berlin-consortium.org/review.html for justification
> >    on the my_ convention, and more notes on meaningful naming.
> >    [[FIXME: should be merged in here at some point.]]
> 
> uh, well, I'v been using the '_' prefix for member variables all over 
> the place. I don't like the idea to have to change that. I wouldn't 
> decorate private methods any special.

Well, you don't necessarily have to change it, just because it's in
this list :-) (Certainly a lot of this stuff (the stuff that can't be
done automatically by indent(1), basically) is almost certainly not
going to be done en masse.)  I'd advocate applying these guidelines to
new code, plus any old code that someone gets around to updating
(maybe because they're working on some particular class anyway, or
because they're insane and decide to convert the whole tree, or
whatever).  Modulo conversion costs, what's your opinion on "my_" vs.
"_"?

> >Actual code
> >-----------
> > - Do not use macros, unless you have permission, in writing, in
> >    triplicate, from Stefan.
> 
> hehe, I like that rule :-)

Thought you might :-)

[snip]
> >Commenting
> >----------
[snip]
> >    Mark code that is used to work around a compiler limitation (like
> >    functionality that is given in the C++ standard to be there but is
> >    missing from your compiler with a comment starting with
> >    WORKAROUND: if this workaround is so ugly that it should be fixed
> >    once a compliant version of your compiler gets out.
> >
> >   Justification: later grepping, etc.  Someone should create a
> >    synopsis plugin that scans the source for these sorts of comments,
> >    puts them on a web-page somewhere, with context and links to the
> >    actual source...
> 
> we should have similar rules for deprecated code (this applies mostly to
> interface level stuff), i.e. we should even think about something that 
> is synopsis-parsable, so people can look up whether some part of an API
> is going to be removed soon, etc.

Nod.  I think the thing to do is fix up the bit of the formatter that
deals with "@" commands, so it actually does something useful :-), and
add a @deprecated tag at that point.  (This is how javadoc does
deprecated things, I believe.)

Note on "do try to remove other people's FIXME's":
> [1] We should discuss how to modify existing code, i.e. who is entitled
>     to modify what. It's more a question of politeness than anything
>     else, but I'd much prefer people to at least try to contact the
>     original author of a piece of code instead of messing themselfs with
>     code they possibly don't fully understan.
>     There have been some cases where someone modified my code in a way
>     that it didn't work any more, or at least it wasn't consistent with
>     my original intention. And even if it works, I think it's just a
>     matter of respect to ask the author (if ever he is still around) to
>     incorporate a modification (or ask for his agreement), instead of
>     just doing it.
> 
>     But that is something everybody has to judge for himself...

If you look over in another part of this thread, you'll see Tobias and
I discussing this.  I'm somewhat reluctant to impose these sorts of
rules.  Time and attention are such scarce resources that making the
overhead for changes larger is... well, not clearly a good idea.  My
inclination is to try to solve this through the social network, rather
than imposed rules -- eg, if you don't understand what something is
doing, you should be asking people who might, no matter what the rules
here say.  And if you're not sure your patch is right, you should be
sending it to the list for comments, not just committing it.  These
are just common sense, and need to be applied flexibly...
Who knows, maybe I'm just too much of an idealist.

> Nathaniel, that's an extremely useful document. Thanks a lot for your work !

Thanks!

-- Nathaniel

-- 
"...these, like all words, have single, decontextualized meanings: everyone
knows what each of these words means, everyone knows what constitutes an
instance of each of their referents.  Language is fixed.  Meaning is
certain.  Santa Claus comes down the chimney at midnight on December 24."
  -- The Language War, Robin Lakoff

This email may be read aloud.