On Mon, Jun 15, 2020 at 4:05 PM Mehdi AMINI via llvm-dev <llvm-dev at lists.llvm.org> wrote:> > As another data point, the MLIR part of the codebase is pretty consistent on this: never use braces for trivial (single statement) if/else/for, but always put it on every branch if needed on any side of the if/else.Any opinion/stance policy/practice on the "one line, or one statement (& possibly comments, etc)" issue?> We also have clang-format pretty heavily enforced (including in the automated pre-merge testing on phabricator) which does not lead to issues where someone would add something into the body of a `for` for example and "forget" to add braces. I don't think I have seen any single instance of such bugs slipping in our code so far. > > > > On Mon, Jun 15, 2020 at 12:46 PM Keane, Erich via llvm-dev <llvm-dev at lists.llvm.org> wrote: >> >> Hi all- >> >> >> >> A few weeks ago I noticed that our “omit braces with single line blocks” rule wasn’t written down! Additionally, as a group on IRC and in review, noticed that the enforcement of this rule has been extremely inconsistent. We made a first run at codifying our existing practice here: https://reviews.llvm.org/D80947, which was then committed after significant time on llvm-commits. >> >> >> >> I would like to encourage the list via discussion and further reviews/commits to come to a consensus on what we actually MEAN by this rule. For example, a recent comment points out that : >> >> >> >> If (cond) >> >> Stmt; >> >> else if (cond) >> >> Stmt; >> >> else { >> >> Stmt; >> >> Stmt; >> >> } >> >> >> >> Should require braces on all of the conditions! However, we are extraordinarily inconsistent here. My wish is for us to become more consistent, so I would like us to use this thread to organize our collective thoughts on figuring out what the rule actually SHOULD be, and organizing a handful of commits to the coding standard to make sure it says what we mean. >> >> >> Thanks, >> Erich >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
On Mon, Jun 15, 2020 at 4:08 PM David Blaikie via llvm-dev < llvm-dev at lists.llvm.org> wrote:> On Mon, Jun 15, 2020 at 4:05 PM Mehdi AMINI via llvm-dev > <llvm-dev at lists.llvm.org> wrote: > > > > As another data point, the MLIR part of the codebase is pretty > consistent on this: never use braces for trivial (single statement) > if/else/for, but always put it on every branch if needed on any side of the > if/else. > > Any opinion/stance policy/practice on the "one line, or one statement > (& possibly comments, etc)" issue? >Generally, any time there is a comment within the body I don't really see it as "trivial" anymore. Prefer: if (...) { // Some comment. single statement; } // Some comment. If (...) single statement; Over: if (...) // Some comment single statement; -- River> > > We also have clang-format pretty heavily enforced (including in the > automated pre-merge testing on phabricator) which does not lead to issues > where someone would add something into the body of a `for` for example and > "forget" to add braces. I don't think I have seen any single instance of > such bugs slipping in our code so far. > > > > > > > > On Mon, Jun 15, 2020 at 12:46 PM Keane, Erich via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> > >> Hi all- > >> > >> > >> > >> A few weeks ago I noticed that our “omit braces with single line > blocks” rule wasn’t written down! Additionally, as a group on IRC and in > review, noticed that the enforcement of this rule has been extremely > inconsistent. We made a first run at codifying our existing practice here: > https://reviews.llvm.org/D80947, which was then committed after > significant time on llvm-commits. > >> > >> > >> > >> I would like to encourage the list via discussion and further > reviews/commits to come to a consensus on what we actually MEAN by this > rule. For example, a recent comment points out that : > >> > >> > >> > >> If (cond) > >> > >> Stmt; > >> > >> else if (cond) > >> > >> Stmt; > >> > >> else { > >> > >> Stmt; > >> > >> Stmt; > >> > >> } > >> > >> > >> > >> Should require braces on all of the conditions! However, we are > extraordinarily inconsistent here. My wish is for us to become more > consistent, so I would like us to use this thread to organize our > collective thoughts on figuring out what the rule actually SHOULD be, and > organizing a handful of commits to the coding standard to make sure it says > what we mean. > >> > >> > >> Thanks, > >> Erich > >> > >> _______________________________________________ > >> LLVM Developers mailing list > >> llvm-dev at lists.llvm.org > >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > > > _______________________________________________ > > LLVM Developers mailing list > > llvm-dev at lists.llvm.org > > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200615/4b332cce/attachment.html>
> On Jun 15, 2020, at 4:13 PM, River Riddle via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > > > On Mon, Jun 15, 2020 at 4:08 PM David Blaikie via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: > On Mon, Jun 15, 2020 at 4:05 PM Mehdi AMINI via llvm-dev > <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: > > > > As another data point, the MLIR part of the codebase is pretty consistent on this: never use braces for trivial (single statement) if/else/for, but always put it on every branch if needed on any side of the if/else. > > Any opinion/stance policy/practice on the "one line, or one statement > (& possibly comments, etc)" issue? > > Generally, any time there is a comment within the body I don't really see it as "trivial" anymore. > > Prefer: > if (...) { > // Some comment. > single statement; > } > // Some comment. > If (...) > single statement; > > Over: > if (...) > // Some comment > single statement;+1 -Chris -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200615/b3e41640/attachment-0001.html>