Page MenuHomePhabricator

[clang] improve accuracy of ExprMutAnalyzer
ClosedPublic

Authored by JonasToth on Sep 22 2020, 5:16 AM.

Details

Summary

This patch extracts the ExprMutAnalyzer changes from https://reviews.llvm.org/D54943
into its own revision for simpler review and more atomic changes.

Diff Detail

Event Timeline

JonasToth created this revision.Sep 22 2020, 5:16 AM
JonasToth retitled this revision from [clang] improve accuracy of ExprMutAnalyzer to WIP [clang] improve accuracy of ExprMutAnalyzer.Sep 22 2020, 8:06 AM
aaron.ballman added inline comments.Sep 23 2020, 5:44 AM
clang/lib/Analysis/ExprMutationAnalyzer.cpp
44

Unless the value -> Matches unless the value

65

I think I'd prefer this to be named hasAnyInit() to complement hasInit() -- these aren't really arguments.

275

operator call expression -> The call operator expression

277

moditication -> modification

345

casted -> cast

465

It is possible, that -> It is possible that

468

is faster to find then all -> is then faster to find all

471

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()?

clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
66

Was there a reason you hoisted this out of the for loop?

JonasToth updated this revision to Diff 293967.Thu, Sep 24, 1:30 AM
  • improve the expression matcher, to catch the expression in general ternary-operator cases, too
  • considered unresolved operator calls to be a modification
  • fix method calls with templates in some instances
JonasToth updated this revision to Diff 294050.Thu, Sep 24, 7:24 AM
JonasToth marked 9 inline comments as done.
  • adress review comments
clang/lib/Analysis/ExprMutationAnalyzer.cpp
471

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.

clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
66

Jup.

buffer.clear()

The current form does proper memory-recycling (i believe at least :D)

aaron.ballman accepted this revision.Thu, Sep 24, 9:34 AM

LGTM aside from a nit.

clang/lib/Analysis/ExprMutationAnalyzer.cpp
471

Ah, okay, that makes a lot more sense, thanks!

clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
66

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.

This revision is now accepted and ready to land.Thu, Sep 24, 9:34 AM
JonasToth marked 2 inline comments as done.Fri, Sep 25, 7:18 AM
JonasToth updated this revision to Diff 294300.Fri, Sep 25, 7:19 AM
  • address review comments
JonasToth updated this revision to Diff 294301.Fri, Sep 25, 7:21 AM
  • fix typo that provided wrong argument to AST building
JonasToth updated this revision to Diff 296546.Tue, Oct 6, 2:32 PM
  • document sections in the testcases, hopefully little more structure for comprehension
  • fix derived-to-base-cast OUTSIDE of an conditional operator
  • improve type-dependent callexpr, too
  • better canResolveToExpr matcher code
  • adjust matcher for comma
JonasToth added a comment.EditedTue, Oct 6, 2:32 PM

@aaron.ballman I update the code a bit. This stuff is just a hydra :/

But i think incrementally the current version is better, but still not perfect. I update the clang-tidy part, too.

Edit: from scrolling through there are still rough edges in the current version. i will remove the stuff tomorrow, right now i am to tired :/

JonasToth retitled this revision from WIP [clang] improve accuracy of ExprMutAnalyzer to [clang] improve accuracy of ExprMutAnalyzer.Tue, Oct 6, 2:32 PM
JonasToth updated this revision to Diff 296628.Wed, Oct 7, 2:41 AM
  • fix imprecisions
aaron.ballman accepted this revision.Thu, Oct 8, 7:30 AM

LGTM though I may have spotted a potential improvement (it can be handled in a follow-up if you want).

clang/lib/Analysis/ExprMutationAnalyzer.cpp
61

Do you also want to handle the binary conditional operator (a GNU extension like a ? : b)?

JonasToth updated this revision to Diff 297187.Fri, Oct 9, 4:15 AM
  • [Feature] add elvis operator detection
JonasToth marked an inline comment as done.Fri, Oct 9, 4:16 AM
JonasToth added inline comments.
clang/lib/Analysis/ExprMutationAnalyzer.cpp
61

Yup, was a fast improvement. Thanks for pointing that out 👍

This revision was landed with ongoing or failed builds.Fri, Oct 9, 4:46 AM
This revision was automatically updated to reflect the committed changes.
JonasToth marked an inline comment as done.