| Re: [cdt-dev] Code Style for CDT code |
On Tue, Jan 27, 2015 at 1:23 PM, Alena Laskavaia <elaskavaia.cdt@xxxxxxxxx> wrote:I personally never had issue with formatter so I use "Format edited lines" auto action in workspace settings.I don't have strong preference between 110 vs 132, any objections about 110?If we override workspace prefs with project prefs for auto-save then people cannot really add any more actions without them appearing in git.Since different save actions are stored under separate preference keys, the effective preferences are determined by the project-level preferences and those workspace-level preferences that are not specified at the project level. We just have to be careful to not overspecify the project-level preferences.-sergeyOn Tue, Jan 27, 2015 at 4:17 PM, Sergey Prigogin <eclipse.sprigogin@xxxxxxxxx> wrote:On Tue, Jan 27, 2015 at 12:33 PM, Alena Laskavaia <elaskavaia.cdt@xxxxxxxxx> wrote:- I will update formatter section to recommend usage of 132 chars per line (same for comments).- 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 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 statementSo my conclusions from this discussion:And it actually looks nicer too with braces in this case - if one branch has braces then another one should too.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.Please make it 100 or 110.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)Import Organizer is pretty harmless and can be enabled in the project-specific settings.If you want to add any other recommendations about code style we can discuss this on this mailing list-sergeyOn 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