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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
