Re: [lmi] A matter of style

[Greg Chicares]

On 2007-3-5 16:27 UTC, Vadim Zeitlin wrote:
> On Thu, 22 Feb 2007 13:00:10 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> I'm still reluctant to endorse
> GC> 
> GC>     if(IsVoid())
> GC>         return;
> GC> 
> GC> without any braces, for the reason given in the footnote below.
> 
>  For this reason to apply you would presumably have to insert a new line
> just before "return" as there is not much sense in adding it below it. And
> to be honest I really don't see how can you forget to add braces then.

Probably we all have some personal blind spots. I think I might
have made this mistake myself; I know I've seen others make it
in lmi code, and it can be painful to track down if it's not
rejected in code review.

My worst blind spot is writing equality comparisons with '='.
Try as I might, I cannot learn to do that correctly. But I have
successfully learned to write constants on the left, and that
lets the compiler save me in most cases.

> I
> can understand how it might happen when you append a line below (you might
> not see the top of the if or something like that) but not when you insert a
> line in the middle.

I was thinking that a simple guideline, expressed in fewer words,
would be better than one with numerous exceptions, even if there
exist cases for which an exception could plausibly be made. (And
the simpler the rule, the more likely it is that we can detect
potential problems automatically someday.)

Nathan Myers's guideline, for instance, seems simple to me:

http://www.cantrip.org/coding-standard2.html
| Dependent clauses of all control structures (if, while, else, case)
| should be blocks. One-line dependent clauses may be represented as
| one-line blocks. A dependent clause starts on a separate line from
| the conditional statement.

I think his rule would permit
  if(x)
    {return;}
but forbid
  if(x)
    return;
.

> GC> This change:
> GC> -        return;
> GC> +        {return;}
> GC> costs only two characters and zero lines. I'll make such changes
> GC> in new code now, and in old code as it's revised.
> 
>  The line "{return;}" has a rather unique indentation/spacing.

Interesting. I tried "google code search" with these criteria:
  "{return;}" lang:c++
  [I couldn't get "^ *{return;}$" to work there]
and found mostly inline functions, like

  wxHtmlCell *GetNext() const {return m_Next;}  

The search turned up some if-statements with a bracketed clause
on the same line (which I personally dislike because the early
returns don't jump out at me), e.g.:

  if (!hdc) {return;}

  if(a == 0) {return;}

  if ( ! queue.empty()) {return;} // queue already done

but not many occurrences of what I was proposing--though I
did find a couple, like this fragment:

  if ("ANYWHERE" == where)
    { return 1; }
  else if ("STRESS" == where)
    { return ((df_.hasStressInfo) && (df_.isStressed (x))) ; }
  else if ("START" == where)
    ...

Even if it's relatively rare usage, is it actually not transparent?

> As this
> change will be done gradually it means that for the foreseeable future such
> lines will be rare and so, at least in the beginning, very surprising. And
> while I don't normally use aesthetic arguments in technical discussions I
> can't hide that personally I think this look very ugly.

Then perhaps we shouldn't do it, and stick with the old way instead:

  if(x)
    {
    return;
    }

> And in any case it
> definitely looks unusual and so attracts attention to this line which is
> undesirable as this line is not important at all: the precondition check is
> not very important on its own (less important than the actual function
> body, that is) and this line is even less so.

Interesting. I liked it precisely because it draws attention to
the early return, and separates it from the more-important code
in the actual function body. But I don't really feel strongly
about this point.

>  So IMHO using just the usual "return" would be better.

Yet then I'd re-raise my original objection.

However, we could delete the middle sentence of Nathan's rule,
or perhaps change it this way:

- One-line dependent clauses may be represented as one-line blocks.
+ Inline function bodies may be represented as one-line blocks.

Alternatively, suppose we found somebody (not necessarily a
programmer) to change all occurrences throughout the code to

  if(x)
    {return;}

all at once, so that we'd have global uniformity. Would you
still see it as making the code harder to read? Let me know:
if you do, then we'll find somebody to split all occurences
onto three lines instead, as before. This little script
  sed -e'/{return;}/s/^\( *\){return;}/\1{\n\1return;\n\1}/'
would come close, though it would change some inline function
bodies that might be better left alone.