ymandel added a comment.
Thanks for the reviews.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:185
+ // base classes, because they are not visible in derived classes.
+ getFieldsFromClassHierarchy(Base.getType(), /*IgnorePrivateFields=*/true,
+ Fields);
----------------
xazax.hun wrote:
> Will this work well for all cases of diamond shape inheritance?
>
> E.g.:
> ```
> struct A { int a; };
> struct B : virtual A { int b; };
> struct C : virtual A { int c; };
> struct D : B, C { int d; };
> ```
>
> In the above code, I would expect the field `a` to appear only once. I guess
> this should work well because of the set representation although we will
> visit A twice.
>
> On the other hand, consider:
> ```
> struct A { int a; };
> struct B : A { int b; };
> struct C : A { int c; };
> struct D : B, C { int d; };
> ```
>
> Here, in fact, we have 2 instances of the field `a`. Both `B::a` and `C::a`
> are part of `D`. I suspect that the current implementation might not handle
> this.
Good point, no. But, I'm not sure that fixing this here would be enough - I
think we'd also need to revisit how we model structs, since `getChild` takes a
decl as argument -- it doesn't support multiple versions of the same field decl.
I added a FIXME since this seems a larger issue, but let me know if you think
it needs fixing now.
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1014
+ std::string Code = R"(
+ struct A {
+ int Bar;
----------------
sgatev wrote:
> Add a similar test with `class` instead of `struct`?
I went with a simpler test for struct, only verifying that the default is
understood correctly (and differently than class), but happy to expand it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122273/new/
https://reviews.llvm.org/D122273
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
