Re: [cdt-dev] Code Style for CDT code

[Marc Dumais <marc.dumais@xxxxxxxxxxxx>]

Title:

+1 sounds reasonable.

On 01/27/2015 03:33 PM, Alena Laskavaia wrote:

Well in this specific example I would say it should be braces. Because nobody would try to stop and think - is it safe or not,
it is easier to just add them. Also I Sergey pointed out it will not increase code length.

And it actually looks nicer too with braces in this case - if one branch has braces then another one should too.

So my conclusions from this discussion:

- I can update cdt code guidance to include statement about usage of braces, I prefer variant, where there is exception for an if with a single then statement

- This considered recommendation and we won't be fighting about it on the review. But if reviewer brings it up the comment should be addressed (with coding style recommendations on cdt wiki page overriding individual committer preferences).

- I will update formatter section to recommend usage of 132 chars per line (same for comments). Again recommendation, only usable in dispute.

- I will add recommendation to use Import Organizer as default save action with default eclipse setting (from our prev. discussions)

If you want to add any other recommendations about code style we can discuss this on this mailing list

On Tue, Jan 27, 2015 at 2:16 PM, Marc Khouzam <marc.khouzam@xxxxxxxxxxxx> wrote:

> We can discuss this on code review, but if we disagree, like in this case, we need to
> discuss this with all committees. Especially if deal is "No I would not remove -1 unless
> you fix the braces".

Agreed.  But to be fair, I didn't say that.  I asked what was your reason to
"strongly disagree" with putting braces.  I also mentioned that we had a problem with
a missing brace before, which is why I was recommending adding them.

It is not ok to say "No I would not remove -1 unless you fix the braces".
But I am comfortable with saying "No I will not remove my -1 unless you answer the
question of the reviewer".

Funny thing is, after talking with Marc Dumais, it turns out I was wrong about
asking for braces in this particular review because it was an if-else :) For
example:

if (a)
  System.out.println();
else {
  a = 8;
  return;
}

There is no risk of adding an extra line after the sysout because the compiler
will know right away it is a mistake (orphaned else statement).  I hadn't
realized that.  But you also didn't tell me :)

> I still don't understand where all these rules are coming from, if there "developers
> guide for dsf" document somewhere which I am missing?

They are not really rules.  They are just recommendations based on experience.
Sometimes they're valuable recommendations, sometimes less so.  But that
is what reviews are for, in part.

[...]

> So every extra bracket you put in which takes separate line reduces amount
> of real code that can fit in the screen which significantly reduce ability of
> another developer to understand the code.

As reviewers, we have to judge what is best in each case.  This above justification
is valid in some cases and I hadn't thought of it. I'll include it in my reviewing 'arsenal' :)
But in some cases (like the bug in the iPhone), it is not worth the risk.

Marc

_______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/cdt-dev

_______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/cdt-dev

--

 

 

 

MARC DUMAIS
Software developer

Ericsson
8500 Decarie
Ville Mount-Royal, Qc, Canada
Phone 514 345-7900 x45006
marc.dumais@xxxxxxxxxxxx
www.ericsson.com