[PATCH] D37264: [Docs] Update CodingStandards to recommend range-based for loops

[Alex Bradbury via Phabricator via llvm-commits]

asb added inline comments.


================
Comment at: docs/CodingStandards.rst:944
 
-  for (BasicBlock::iterator II = BB->begin(), E = BB->end(); II != E; ++II) {
-    if (BinaryOperator *BO = dyn_cast<BinaryOperator>(II)) {
+  for (auto &I : BB) {
+    if (auto *BO = dyn_cast<BinaryOperator>(&I)) {
----------------
asb wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > asb wrote:
> > > > majnemer wrote:
> > > > > javed.absar wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > majnemer wrote:
> > > > > > > > I'd replace the `auto` with `Instruction &`.
> > > > > > > We should clarify this in the coding standard, because I've always understood we generally *want* to use `auto` in range-based for loops, for brevity (and because the type is generally immediately apparent from the container).
> > > > > > In some complicated cases not using 'auto' improves code understanding. I have had cases where well established reviewers of llvm have recommended me not to  use 'auto' in their review comment.
> > > > > The most consistent rule I have seen is that we want to avoid repeating ourselves. `auto` makes a lot of sense if you just made a `std::vector<int>` a few lines up and are now iterating. If you have some `FancyContainerTy`, it is less awesome to use `auto` as it is not clear what it is you get from iterating it.
> > > > For `FancyContainerTy` sure, but my impression is it's quite common to use auto for simple container types where the types are well known to the average LLVM developer. e.g. Instructions in a BasicBlock, BasicBlocks in a Function.
> > > Agreed that we use `auto` primarily when the type is explicitly spelled out in the initialization. I was mostly interested in clarification around range-based for loops. The above is an example where I usually recommend `auto` because I thought that was the direction we had picked back when the C++11 discussion happened. If that's an incorrect understanding, I think we really should clarify the coding standard so I have something more concrete to point to.
> > > For FancyContainerTy sure, but my impression is it's quite common to use auto for simple container types where the types are well known to the average LLVM developer. e.g. Instructions in a BasicBlock, BasicBlocks in a Function.
> > 
> > That's been my recommendation during coding reviews, which is why I was surprised to see the opposite suggested here.
> I was trying to comment specifically on range-based for loops. My comment should have said "it's quite common to use auto for iterating over simple container types...". It could obviously be argued that instances in the codebase that do this shouldn't be doing so.
[Sorry, I got confused about who Aaron was responding to].


https://reviews.llvm.org/D37264