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