[PATCH] D53206: Allow padding checker to traverse simple class hierarchies

[Ben Craig via Phabricator via cfe-commits]

bcraig added a comment.

add a comment, and it will be fine in my eyes.  You'll need signoff from the code owner though.



================
Comment at: test/Analysis/padding_cpp.cpp:204-212
+// expected-warning at +1{{Excessive padding in 'struct FakeIntSandwich'}}
+struct FakeIntSandwich {
+  char c1;
+  int i;
+  char c2;
+};
+
----------------
tekknolagi wrote:
> tekknolagi wrote:
> > bcraig wrote:
> > > Looking at this again... what new cases does this catch?  FakeIntSandwich was caught before (it is identical to 'PaddedA', and AnotherIntSandwich generated no warning before.  So what is different?
> > > 
> > > EBO1 and EBO2 are cases above that would be nice to catch, but aren't being caught right now.
> > Ah, you're right. I'll just keep the one in `padding_inherit`.
> @bcraig I updated the description of the diff to be more informative about the particular cases this change catches.
ok, got it.  Both the old and new will catch the exact same class / struct declarations, but the new checker will catch more array cases.


================
Comment at: test/Analysis/padding_inherit.cpp:2-4
+
+// A class that has no fields and one base class should visit that base class
+// instead.
----------------
tekknolagi wrote:
> bcraig wrote:
> > Evil test case to add...
> > 
> > ```
> > struct Empty {};
> > struct DoubleEmpty : Empty {
> >     Empty e;
> > };
> > ```
> > 
> > I don't think that should warn, as you can't reduce padding by rearranging element.
> > 
> > I guess your new code shouldn't catch that at all anyway, as you are testing in the other direction.... when field-less inherits from field-ed.
> > 
> I'll add that test case -- certainly something to consider when somebody tackles the harder field/inheritance problem.
The comment needs some extra exposition.  AllowedPad is set to 20, yet this gets flagged anyway.  You should mention that this only warns because of the later declaration of `AnotherIntSandwich ais[100];`


Repository:
  rC Clang

https://reviews.llvm.org/D53206