[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang marked 5 inline comments as done.
shuaiwang added inline comments.


================
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:67
+    if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) {
+      if (DRE->getNameInfo().getAsString()[0] == 'p')
+        Finder = PointeeMutationFinder;
----------------
JonasToth wrote:
> That doesn't look like a super idea. It is super hidden that variable 'p*' will be analyzed for pointee mutation and others aren't. Especially in 12 months and when someone else wants to change something, this will be the source of headaches.
> 
> Don't you think there could be a better way of doing this switch?
Yeah... agree :)
But how about let's keep it this way just for now, and I'll pause the work on supporting pointee analysis and fix it properly but in a separate diff following this one?

I've been thinking about this and also did some prototyping, and here are my thoughts:
In order to properly fix this we need to change the interface of `ExprMutationAnalyzer` to not simply return a `Stmt` but instead return a `MutationTrace` with additional metadata. This wasn't needed before because we can reconstruct a mutation chain by continuously calling `findMutation`, and that works well for cases like "int x; int& r0 = x; int& r1 = r0; r1 = 123;". But now that pointers come into play, and we need to handle cases like "int *p; int *p2 = p; int& r = *p2; r = 123;", and in order to reconstruct the mutation chain, we need to do `findPointeeMutation(p)` -> `findPointeeMutation(p2)` -> `findMutation(r)`, notice that we need to switch from calling `findPointeeMutation` to calling `findMutation`. However we don't know where the switch should happen simply based on what's returned from `findPointeeMutation`, we need additional metadata for that.

That interface change will unavoidably affect how memoization is done so we need to change memoization as well.

Now since we need to change both the interface and memoization anyway, we might as well design them properly so that multiple-layers of pointers can be supported nicely. I think we can end up with something like this:
```
class ExprMutationAnalyzer {
public:
  struct MutationTrace {
    const Stmt *Value;
    const MutationTrace *Next;
    // Other metadata
  };

  // Finds whether any mutative operations are performed on the given expression after dereferencing `DerefLayers` times.
  const MutationTrace *findMutation(const Expr *, int DerefLayers = 0);
  const MutationTrace *findMutation(const Decl *, int DerefLayers = 0);
};
```
This involves quite some refactoring, and doing this refactoring before this diff and later rebasing seems quite some trouble that I'd like to avoid.

Let me know what do you think :)


Repository:
  rC Clang

https://reviews.llvm.org/D52219