xazax.hun added inline comments.
================
Comment at:
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:120
- // FIXME: Add `StorageLocation` for `this`.
+ StorageLocation *ThisPointeeLoc = nullptr;
----------------
It feels a bit wrong to have a separate `ThisPointeeLoc` here. But as far as I
understand, this is an artifact of the AST representation we have. I think a
superior AST representation would have an implicit `ParmVarDecl` to represent
`this`, so we would not need to do any special handling at all, the general
code path would do the right thing. I think the C++ language, with the
`deducing this` started its approach in this direction officially. I really
hope someone will have the time to fix all the mess and get rid of all the
unnecessary corner cases in the foreseeable future :)
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:52
+ setStorageLocation(*ParamDecl, ParamLoc);
+ initValueInStorageLocation(ParamLoc, ParamDecl->getType());
+ }
----------------
There might be an optimization opportunity for `initValueInStorageLocation`. If
we initialize a class or struct, we will end up create locations for all of the
fields recursively (unless self-referential) right? Actually, in many of the
codebases we cannot even access many of those fields because they might be
private/protected etc. Unfortunately, it might not be easy to query whether a
field is accessible from this method, but I wonder if it is worth a TODO
somewhere.
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:131
+
+ auto &Loc = Env.createStorageLocation(*S);
+ Env.setStorageLocation(*S, Loc);
----------------
Nit: I got confused for a second what will happen in a loop. I wonder if
`createStorageLocation` is better renamed to `createOrGetStorageLocation` to
express the fact it will not always create a new location. But I don't have
strong feelings about this, also feel free to defer to a later PR.
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:141-142
+
+ if (Member->isFunctionOrFunctionTemplate())
+ return;
+
----------------
I wonder if we also want to create a non-null pointer value for these in the
future so we can evaluate certain if statements.
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:813
+
+ void target(A &Foo) {
+ (void)0;
----------------
I wonder if we can make the tests a bit more concise by merging some of them.
E.g. we could have a single test with both a pointer, a reference, and a value
param. Although I understand that some people like to keep most tests minimal,
so feel free to ignore this.
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1062
+ class A {
+ int Bar;
+
----------------
I'd love to see a test with multiple fields and a nested struct.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117012/new/
https://reviews.llvm.org/D117012
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
