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