aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM aside from a nit.
================
Comment at: clang/lib/Analysis/ExprMutationAnalyzer.cpp:455
+ const auto HasAnyNonConstIterator =
+ anyOf(allOf(hasMethod(allOf(hasName("begin"), unless(isConst()))),
+ unless(hasMethod(allOf(hasName("begin"), isConst())))),
----------------
JonasToth wrote:
> aaron.ballman wrote:
> > Do we want to look for methods that end with `_?[Bb]egin` or `_?[Ee]nd` so
> > that this would catch patterns like `foo_begin()`/`foo_end()`,
> > `FooBegin()`/`FooEnd()`, or `Foo_Begin()`/`Foo_End()`?
> This specific matcher is only applied in range-for contexts. There only the
> `begin(); end()` methods matter. I updated the comment above to clarify this.
Ah, okay, that makes a lot more sense, thanks!
================
Comment at: clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp:65
+
+ std::string buffer;
for (const auto *E = selectFirst<Expr>("expr", Results); E != nullptr;) {
----------------
JonasToth wrote:
> aaron.ballman wrote:
> > Was there a reason you hoisted this out of the `for` loop?
> Jup.
> ```
> buffer.clear()
> ```
> The current form does proper memory-recycling (i believe at least :D)
This smells like a micro-optimization to me (I think declaring the variable in
the loop is more clear than clearing the buffer on each iteration).
I don't insist on changing it, but if you want to keep it, you should name it
`Buffer` per our usual coding rules.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88088/new/
https://reviews.llvm.org/D88088
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
