sgatev added a comment.
Thank you both for the reviews!
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:52
+ setStorageLocation(*ParamDecl, ParamLoc);
+ initValueInStorageLocation(ParamLoc, ParamDecl->getType());
+ }
----------------
xazax.hun wrote:
> 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.
Right. I added a FIXME in `initValueInStorageLocationUnlessSelfReferential`.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:57
+ if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(&DeclCtx)) {
+ if (!MethodDecl->isStatic()) {
+ QualType ThisPointeeType = MethodDecl->getThisObjectType();
----------------
ymandel wrote:
> nit: invert the if and return?
I prefer to keep the block self-contained as we might end up adding code after
the outer if statement.
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:131
+
+ auto &Loc = Env.createStorageLocation(*S);
+ Env.setStorageLocation(*S, Loc);
----------------
xazax.hun wrote:
> 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.
Ack. I added a FIXME.
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:141-142
+
+ if (Member->isFunctionOrFunctionTemplate())
+ return;
+
----------------
xazax.hun wrote:
> 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.
I added a FIXME.
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:813
+
+ void target(A &Foo) {
+ (void)0;
----------------
xazax.hun wrote:
> 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.
I'll keep the tests separate for now, but I'll think about how to make them
more concise.
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:919
+
+ EXPECT_EQ(Env.getValue(*BazDecl, SkipPast::None), BarVal);
+ });
----------------
ymandel wrote:
> Why EXPECT here but ASSERT (as last line) in previous tests?
My bad. I updated all tests to use ASSERT only where necessary.
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1059
+
+TEST_F(TransferTest, ClassThisMember) {
+ std::string Code = R"(
----------------
ymandel wrote:
> why test a class distinct from a struct? Is there a meaningful difference
> between the two with regard to our model? if so, might it be worth instead
> testing explicit public vs private?
For completeness. Changing the `isStructureOrClassType` predicate in
`initValueInStorageLocationUnlessSelfReferential` to `isStructureType` or
`isClassType` should make one of these tests fail. I'll address public and
private members in a follow up.
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1062
+ class A {
+ int Bar;
+
----------------
xazax.hun wrote:
> I'd love to see a test with multiple fields and a nested struct.
I added a nested struct and an extra member.
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
