[PATCH] D122273: [clang][dataflow] Fix handling of base-class fields

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.


================
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);
----------------
ymandel wrote:
> 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.
I think the argument of `getChild` needs fixing as well is strong enough to push this to a separate PR.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122273/new/

https://reviews.llvm.org/D122273