The CodingStandards section on avoiding the re-evaluation of end() hasn't been updated since range-based for loops have been adopted in the LLVM codebase. This patch adds a very brief section that documents how range-based for loops should be used wherever possible. It also moves example code in CodingStandards to use range-based for loops and auto when appropriate.
Details
Diff Detail
Event Timeline
docs/CodingStandards.rst | ||
---|---|---|
944–945 | 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). |
docs/CodingStandards.rst | ||
---|---|---|
944–945 | 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. |
docs/CodingStandards.rst | ||
---|---|---|
944–945 | 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. |
docs/CodingStandards.rst | ||
---|---|---|
944–945 | 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. |
docs/CodingStandards.rst | ||
---|---|---|
944–945 | 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. |
docs/CodingStandards.rst | ||
---|---|---|
944–945 |
That's been my recommendation during coding reviews, which is why I was surprised to see the opposite suggested here. |
docs/CodingStandards.rst | ||
---|---|---|
944–945 | 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. |
docs/CodingStandards.rst | ||
---|---|---|
944–945 | [Sorry, I got confused about who Aaron was responding to]. |
As requested by @majnemer, this updated version of the patch adds in some more explanation about re-evaluating end() and mutation.
FWIW, I agree with @majnemer's comments here: use of auto in those places doesn't aid in understanding/maintainability of the code, and it is important to mention that the manual loop with an explicitly evaluated end can make sense if the sequence is being mutated in the loop (but also we should recommend a comment to make that explicit!)
-Chris
Thank you everybody for the feedback. I've made several updates vs the previous version of the patch:
- Don't use auto for the element type in range-based for loops
- Retain most of the previous text on evaluating end() every time for a loop. I initially thought I could relegate this to a shorter note given that explicit iterator-based loops should be relatively rare, but my attempt to do this lost important points.
- Do use auto for the iterator (hopefully this is uncontroversial, as it is explicitly suggested in the "Use `auto` Type Deduction to Make Code More Readable" section
I'd replace the auto with Instruction &.