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

[Max Bernstein via Phabricator via cfe-commits]

tekknolagi added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:167-171
     }
+    // How do you reorder fields if you haven't got any?
+    else if (RD->field_empty())
+      return true;
+
----------------
bcraig wrote:
> I think this should just be an if, and not an else if.
Changing it to a normal if mucks with the fall-through if pattern: in the old behavior, any empty struct would be skipped. In this new behavior, it shouldn't be the same -- it should only be the empty structs that don't have any base classes, etc.


================
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.
----------------
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.


Repository:
  rC Clang

https://reviews.llvm.org/D53206