[PATCH] D14779: Adding checker to detect excess padding in records

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

> I have seen plenty of structures where the specific layout was important and couldn't be changed.


Can you give specific examples of these? Can we develop heuristics for them?

> These generally felt like noisy reports unless I had more specific justification for them (like evidence of 

>  an array of the elements). Should it be higher? As I get better at detecting arrays, then I think it makes 

>  sense to bump the raw value higher.


I think it's better to report many fewer issues that are real problems to "advertise" the checker. Once people see it's value, they can lower the threshold. If we report hundreds of issues, it will scare people off.


================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:72
@@ +71,3 @@
+    if (shouldSkipDecl(RD))
+      return;
+
----------------
That's true, but it does not matter for the syntactic checkers much.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:145
@@ +144,3 @@
+    }
+    auto IsTrickyField = [](const FieldDecl *FD) -> bool {
+      // Bitfield layout is hard.
----------------
I wonder why the analyzer diagnostic reporting mechanism does not take care of this.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:322
@@ +321,2 @@
+  Mgr.registerChecker<PaddingChecker>();
+}
----------------
I think it is important to check the numbers to make sure that logic does not regress. Maybe you could create one clone for x86 or only test on x86? Is testing on each architecture tests the code you wrote?


http://reviews.llvm.org/D14779