[PATCH] D82594: Clarify a bit the guideline on omitting braces, including more examples (NFC)

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/docs/CodingStandards.rst:1657
+  if (auto *D = dyn_cast<FunctionDecl>(D)) {
+    if(shouldProcess(D))
+      handleVarDecl(D);
----------------
MaskRay wrote:
> mehdi_amini wrote:
> > lattner wrote:
> > > mehdi_amini wrote:
> > > > MaskRay wrote:
> > > > > lattner wrote:
> > > > > > mehdi_amini wrote:
> > > > > > > lattner wrote:
> > > > > > > > MaskRay wrote:
> > > > > > > > > Add a space after `if`
> > > > > > > > and after the for.  Also, I'd recommend using size_t instead of 'int', and != instead of <.  
> > > > > > > Why the `size_t` here? I thought best practice was rather to write for loops with `int`?
> > > > > > size_t is technically correct and the best type to use.  LLVM historically has used 'unsigned' for most counted loops (this was my fault) which isn't great for induction variable optimizations on 64-bit machines.  "int" is better for 64-bit machines because UB-on-overflow permits the important transformations, but this is technically incorrect and there is no obvious reason to prefer it.
> > > > > > "int" is better for 64-bit machines because UB-on-overflow permits the important transformations, but this is technically incorrect and there is no obvious reason to prefer it.
> > > > > 
> > > > > An unsigned index might be relevant for 16-bit computers. When we come to 32-bit and 64-bit era, ability to index more than half of the memory seems pretty irrelevant... Defined modulo 2 behavior impedes optimization and breeds bugs like `for (size_t i = 0; i < a.size() - 1; ++i)`. Using int is fine, though I acknowledge that unsigned has been used a lot in LLVM's code base. C++20 even introduced std::ssize to make signed indexes more of "first-class" (I believe it is http://wg21.link/p1227r2 )
> > > > > size_t is technically correct and the best type to use. 
> > > > 
> > > > How so? This contradicts most of what I read in "recent" C++ talk / publications, I summarized it here: http://lists.llvm.org/pipermail/llvm-dev/2019-June/132890.html
> > > size_t is technically the correct type to use in all ways for an induction variable that starts at 0 and counts up.  If you'd like to discuss this in depth, please (re) raise the discussion on llvm-dev.  I can see some theoretical arguments for ssize_t, but int is inferior in every way I can see.
> > > 
> > > Thanks!
> > > size_t is technically the correct type to use in all ways for an induction variable that starts at 0 and counts up. 
> > 
> > It seems to me that you're repeating yourself here, I'd be happy to be convinced but right now it does not seem that you're really providing me with actual information? 
> > I actually *already* brought up the topic on llvm-dev, as I linked in my previous comment, did you miss it?
> > My llvm-dev email is sourced and includes lengthy explanations, and Chandler also motivated it here: http://lists.llvm.org/pipermail/llvm-dev/2019-June/133023.html
> > 
> > The thread didn't build consensus one way or another, but most of the contributors voting for "unsigned" in the thread seemed to me to express a "personal preference" and I have yet to see a good technical explanation why  `size_t` would be the obvious correct choice? 
> > The thread didn't build consensus one way or another, but most of the contributors voting for "unsigned" in the thread seemed to me to express a "personal preference" and I have yet to see a good technical explanation why size_t would be the obvious correct choice?
> 
> Since I work on binary utilities/ELF a lot with James & Jake (who favored "unsigned"), I can probably comment on their preference. I agree that in ELF it is common to use an unsigned type: it is mostly because the binary formats use unsigned types a lot: `Elf32_Section`. In that area, an unsigned index does make sense. For many other loops in other areas where there is no strong preference for an unsigned index, to break the tie, a signed type does work better ((1) optimization (2) gotcha of algebra near zero). Zachary's comment seems to be more about concern about using an unsigned index in an area where it does not fit in. However, as a "breaking the tie" rule, I have a strong +1 to preferring unsigned.
> 
> Anyhow, the updated revision sidesteps the potentially controversial topic by using `for (int i : llvm::seq<int>(count))`
> 
> However, as a "breaking the tie" rule, I have a strong +1 to preferring unsigned.

Sorry, typo, I of course mean a strong +1 to preferring a signed type :) (unsigned is problematic and many C++ standard committee members consider it "wrong". Many other languages (e.g. Swift) use Int as well :) )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82594/new/

https://reviews.llvm.org/D82594