[llvm-commits] [llvm] r76723 - /llvm/trunk/docs/CodingStandards.html

[Daniel Dunbar]

Thanks!

Some comments:

On Tue, Jul 21, 2009 at 10:41 PM, Chris Lattner<sabre at nondot.org> wrote:
> +<!-- _______________________________________________________________________ -->
> +<div class="doc_subsubsection">
> +  <a name="hl_predicateloops">Turn Predicate Loops into Predicate Functions</a>
> +</div>
> +
> +<div class="doc_text">
> +
> +<p>It is very common to write inline functions that just compute a boolean
> +   value.  There are a number of ways that people commonly write these, but an
> +   example of this sort of thing is:</p>

This doesn't make sense, I presume you meant something like "It is
very common to write small loops ..."

> Code like this would be preferred:

We prefer...?

> +<p>
> +In general, we strive to reduce indentation where ever possible.  This is useful
> +because we want code to <a href="#scf_codewidth">fit into 80 columns</a> without
> +wrapping horribly, but also because it makes it easier to understand the code.
> +Namespaces are a funny thing: they are often large, and we often desire to put
> +lots of stuff into them (so they can be large).  Other times they are tiny,
> +because they just hold an enum or something similar.  In order to balance this,
> +we use different approaches for small versus large namespaces.
> +</p>
> +
> +<p>
> +If a namespace definition is small and <em>easily</em> fits on a screen (say,
> +less than 35 lines of code), then you should indent its body.  Here's an
> +example:
> +</p>

I medium-strongly dislike this choice of style. :)

A personal "guideline" for style guidelines is that they should be
nearly automatic; a guideline that is easier to apply is more likely
to be followed consistently, and in then end I prefer consistency over
any particular style choice.

This guideline is hard to follow because when writing a class you
can't normally predict its size; so you don't know the indentation.
Similarly, in practice this guideline will end up being frequently not
followed, as code changes and whether it fits "on screen" changes.
Your editor is also not particularly going to help you apply it.

If the strongest motivation is reduce indentation, then I see little
value in making the distinction, we could just never indent
namespaces. I think the "small" case only applies in a few scenarios,
and indenting them adds little value.

>  <div class="doc_code">
>  <pre>
> -std::cout << std::endl;
> -std::cout << '\n' << std::flush;
> +/// SomeCrazyThing - This namespace contains flags for ...
> +namespace SomeCrazyThing {
> +  enum foo {
> +    /// X - This is the X flag, which is ...
> +    X = 1,
> +
> +    /// Y - This is the Y flag, which is ...
> +    Y = 2,
> +
> +    /// Z - This is the Z flag, which is ...
> +    Z = 4,
> +
> +    /// ALL_FLAGS - This is the union of all flags.
> +    ALL_FLAGS = 7
> +  };
> +}
>  </pre>
>  </div>

This in itself is frequently bad style, because such enumerations can
usually be tied to a class, with a shorter type name & less namespace
pollution. Sure, we need them once in a while, but its not a common
case.

> -<p>Most of the time, you probably have no reason to flush the output stream, so
> -it's better to use a literal <tt>'\n'</tt>.</p>
> +<p>Since the body is small, indenting adds value because it makes it very clear
> +where the namespace starts and ends, and it is easy to take the whole thing in
> +in one "gulp" when reading the code.  If the blob of code in the namespace is
> +larger (as it typically is in a header in the llvm or clang namespaces), do not
> +indent the code, and add a comment indicating what namespace is being closed.
> +For example:</p>
> +
> +<div class="doc_code">
> +<pre>
> +namespace llvm {
> +namespace knowledge {
> +
> +/// Grokable - This class represents things that Smith can have an intimate
> +/// understanding of and contains the data associated with it.
> +class Grokable {
> +...
> +public:
> +  explicit Grokable() { ... }
> +  virtual ~Grokable() = 0;
> +
> +  ...
> +
> +};
> +
> +} // end namespace knowledge
> +} // end namespace llvm
> +</pre>
> +</div>
> +
> +<p>Because the class is large, we don't expect that the reader can easily
> +understand the entire concept in a glance, and the end of the file (where the
> +namespaces end) may be a long ways away from the place they open.  As such,
> +indenting the contents of the namespace doesn't add any value, and detracts from
> +the readability of the class.  In these cases it is best to <em>not</em> indent
> +the contents of the namespace.</p>

I think this argument is specious -- both because I've never felt
indenting a namespace hurt readability (although I agree this could
happen if we used many levels of namespaces since it would collide
with the 80-column rule), and because one is generally aware of the
enclosing indentation layer by virtue of the indentation of method
names, etc.

Put another way, we always indent in class definitions, and it doesn't
hurt readability.

> +
> +</div>
> +
> +<!-- _______________________________________________________________________ -->
> +<div class="doc_subsubsection">
> +  <a name="micro_anonns">Anonymous Namespaces</a>
> +</div>
> +
> +<div class="doc_text">
> +
> +<p>A common topic after talking about namespaces is anonymous namespaces.

This is a painful sentence. :)

> +Also, there is no reason to enclose the definition of "operator<" in the
> +namespace since it was declared there.

s/since/just because/" ?

 - Daniel