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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Patch looks great, thanks!



================
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;
----------------
This check is already in `shouldSkipDecl()` (?)


================
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.
----------------
I guess the TODO is still kinda partially relevant, eg. "TODO: support other combinations of base classes and fields"?


================
Comment at: test/Analysis/padding_inherit.cpp:20
+
+AnotherIntSandwich ais[100];
+
----------------
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.


Repository:
  rC Clang

https://reviews.llvm.org/D53206