Page MenuHomePhabricator

[clang] improve accuracy of ExprMutAnalyzer

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



This patch extracts the ExprMutAnalyzer changes from
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

Unless the value -> Matches unless the value


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


operator call expression -> The call operator expression


moditication -> modification


casted -> cast


It is possible, that -> It is possible that


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


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


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

JonasToth updated this revision to Diff 293967.Sep 24 2020, 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.Sep 24 2020, 7:24 AM
JonasToth marked 9 inline comments as done.
  • adress review comments

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.




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

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

LGTM aside from a nit.


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


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.Sep 24 2020, 9:34 AM
JonasToth marked 2 inline comments as done.Sep 25 2020, 7:18 AM
JonasToth updated this revision to Diff 294300.Sep 25 2020, 7:19 AM
  • address review comments
JonasToth updated this revision to Diff 294301.Sep 25 2020, 7:21 AM
  • fix typo that provided wrong argument to AST building
JonasToth updated this revision to Diff 296546.Oct 6 2020, 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.EditedOct 6 2020, 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.Oct 6 2020, 2:32 PM
JonasToth updated this revision to Diff 296628.Oct 7 2020, 2:41 AM
  • fix imprecisions
aaron.ballman accepted this revision.Oct 8 2020, 7:30 AM

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


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

JonasToth updated this revision to Diff 297187.Oct 9 2020, 4:15 AM
  • [Feature] add elvis operator detection
JonasToth marked an inline comment as done.Oct 9 2020, 4:16 AM
JonasToth added inline comments.

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

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