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@+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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
