Page MenuHomePhabricator

[Docs] Update CodingStandards to recommend range-based for loops
ClosedPublic

Authored by asb on Aug 29 2017, 8:18 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

asb created this revision.Aug 29 2017, 8:18 AM
majnemer added inline comments.
docs/CodingStandards.rst
944 ↗(On Diff #113097)

I'd replace the auto with Instruction &.

964 ↗(On Diff #113097)

Ditto.

1338 ↗(On Diff #113097)

You removed text which mentioned the most common case was mutation while iteration, I think we should keep at least part of that exposition somewhere.

aaron.ballman added inline comments.
docs/CodingStandards.rst
944 ↗(On Diff #113097)

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).

javed.absar added inline comments.
docs/CodingStandards.rst
944 ↗(On Diff #113097)

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.

majnemer added inline comments.Aug 29 2017, 10:13 AM
docs/CodingStandards.rst
944 ↗(On Diff #113097)

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.

asb added inline comments.Aug 29 2017, 10:25 AM
docs/CodingStandards.rst
944 ↗(On Diff #113097)

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.

aaron.ballman added inline comments.Aug 29 2017, 10:28 AM
docs/CodingStandards.rst
944 ↗(On Diff #113097)

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.

aaron.ballman added inline comments.Aug 29 2017, 10:32 AM
docs/CodingStandards.rst
944 ↗(On Diff #113097)

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.

asb added inline comments.Aug 29 2017, 10:33 AM
docs/CodingStandards.rst
944 ↗(On Diff #113097)

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.

asb added inline comments.Aug 29 2017, 10:35 AM
docs/CodingStandards.rst
944 ↗(On Diff #113097)

[Sorry, I got confused about who Aaron was responding to].

asb updated this revision to Diff 113117.Aug 29 2017, 10:48 AM
asb marked an inline comment as done.

As requested by @majnemer, this updated version of the patch adds in some more explanation about re-evaluating end() and mutation.

lattner edited edge metadata.Aug 29 2017, 9:23 PM

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

asb updated this revision to Diff 113245.Aug 30 2017, 5:49 AM
asb marked 10 inline comments as done.
asb edited the summary of this revision. (Show Details)

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
lattner accepted this revision.Aug 30 2017, 10:00 PM

LGTM! Thanks @asb

This revision is now accepted and ready to land.Aug 30 2017, 10:00 PM
This revision was automatically updated to reflect the committed changes.