[PATCH] D83351: [llvm-reduce] Reducing attributes

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D83351#2139818 <https://reviews.llvm.org/D83351#2139818>, @lebedev.ri wrote:

> In D83351#2139769 <https://reviews.llvm.org/D83351#2139769>, @dblaikie wrote:
>
> > In D83351#2139751 <https://reviews.llvm.org/D83351#2139751>, @lebedev.ri wrote:
> >
> > > There's also `llvm::for_each()` with 40 uses.
> >
> >
> > Still a very tiny fraction of all iterations. I expect most (or at least an order of magnitude or two more than the <100 for_each()s) of those iterations don't depend on the order of iteration - so I don't think it provides the signal you're thinking of, at least not to enough other developers to be useful - making the code quirky/different in a way that I think is likely to be confusing to other readers ("Why isn't this a range-based for loop? is there some subtle difference in behavior that the author intended that I'm not understanding?" & there isn't)
> >
> > > Can you please quote specific part of the whatever documentation you believe dictates this?
> > >  If there isn't one, i'd like to see `ProgrammersManual` patch.
> >
> > Perhaps this is sufficient: https://llvm.org/docs/CodingStandards.html#use-range-based-for-loops-wherever-possible
>
>
> I'm afraid it is not. It only speaks about range-based for loop vs. old-style for loops.
>  If `for_each` is so bad, we really should proactively document it, so i'm still waiting on the link/patch.
>  Thanks.


I don't think this is an LLVM-specific C++ stylistic issue & there's lots of ways that we could write less than ideal code that we don't document in the style guide. (general guidance like "don't use dynamic allocation when static allocation will do" for instance) & I think this fits under that kind of category.

The existing uses of llvm::for_each look like mostly cases where an existing lambda is used in a few different places and it's slightly shorter/easier to use std::for_each than a range-based-for. I think those are OK/wouldn't object to them. But in these cases where a lambda expression is being passed immediately to for_each, it doesn't seem beneficial to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83351