[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:78-81
+    // We need to be looking at a definition, not just any pointer to the
+    // declaration.
+    if (!(RD = RD->getDefinition()))
+      return;
----------------
NoQ wrote:
> This check is already in `shouldSkipDecl()` (?)
Oh yes, you're right.


================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:83-85
+    // This is the simplest correct case: a class with no fields and one base
+    // class. Other cases are more complicated because of how the base classes
+    // & fields might interact, so we don't bother dealing with them.
----------------
NoQ wrote:
> I guess the TODO is still kinda partially relevant, eg. "TODO: support other combinations of base classes and fields"?
Good point, I'll add that.


================
Comment at: test/Analysis/padding_inherit.cpp:20
+
+AnotherIntSandwich ais[100];
+
----------------
NoQ wrote:
> Now that's actually interesting: i didn't realize that this checker displays warnings depending on how the structure is *used*. The warning doesn't mention the array, so the user would never figure out why is this a true positive. I guess it'd be great to add an extra note (as in `BugReport::addNote()`) to this checker's report that'd be attached to the array's location in the code and would say something like `note: 'struct FakeIntSandwich' is used within array 'ais' with 100 elements`. And also `note: 'struct AnotherIntSandwich' inherits from 'struct FakeIntSandwich'` at the base specifier.
> 
> It's not blocking this patch, just thinking aloud about QoL matters.
I didn't know of this functionality. I like your suggestion!


Repository:
  rC Clang

https://reviews.llvm.org/D53206