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

Anna Zaks via cfe-commits cfe-commits at lists.llvm.org
Wed Nov 18 21:53:28 PST 2015


zaks.anna added a comment.

This is a partial review. I did not look at the padding calculations closely.

Have you run this over codebases other than clang? Are there any false positives?

> Even with the default of 8, this checker is too noisy to justify turning on by default. Clang+LLVM has 

>  hundreds of violations.


How did you pick the default? Should it be higher? 
My main concern is that if the checker is too noisy on most codebases, people might just try it once and keep it turned off. Having a higher default will report less issues but they will be more interesting.


================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:11
@@ +10,3 @@
+//  This file defines a checker that checks for padding that could be
+//  removed by re-ordering members
+//
----------------
Nit: Please, use proper punctuation in comments (in the implementation and test files).

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:33
@@ +32,3 @@
+//===----------------------------------------------------------------------===//
+// PaddingChecker
+//===----------------------------------------------------------------------===//
----------------
This comment is redundant.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:48
@@ +47,3 @@
+    struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
+      const PaddingChecker *Self;
+      BugReporter &BR;
----------------
'Self' is a bit confusing since it's not LocalVisitor but the PaddingChecker class.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:56
@@ +55,3 @@
+          : Self(Self), BR(BR), AllowedPad(AllowedPad) {}
+      bool VisitRecordDecl(const RecordDecl *RD) {
+        Self->visitRecord(RD, BR, AllowedPad);
----------------
Why do we need to implement the visitation here? Can PaddingChecker just implement checkASTDecl(const RecordDecl *RD, ...) and checkASTDecl(const VarDecl *RD, ...)?

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:71
@@ +70,3 @@
+
+  void visitRecord(const RecordDecl *RD, BugReporter &BR,
+                   int64_t AllowedPad) const {
----------------
I'd just make the BugReporter and AllowedPad members to avoid passing them around.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:88
@@ +87,3 @@
+      if (DiffPad.isNegative()) {
+        llvm_unreachable("OptimalPad was somehow larger than the BaselinePad");
+      }
----------------
This would be better expressed with assert(!DiffPad.isNegative() && "DiffPad should not be negative"); 

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:97
@@ +96,3 @@
+  // Look for arrays of overly padded types.  If the padding of the
+  // array type exceeds AllowedPad, then generate a report.
+  void visitVariable(const VarDecl *VD, BugReporter &BR,
----------------
nit: Please, use doxygen style comments.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:114
@@ +113,3 @@
+      return;
+    auto &ASTContext = RD->getASTContext();
+    const ASTRecordLayout &RL = ASTContext.getASTRecordLayout(RD);
----------------
Should the rest of the function implementation be moved into a subroutine? Looks like copy and paste from visitRecord.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:144
@@ +143,3 @@
+        BR.getSourceManager().getFileCharacteristic(Location);
+    // Throw out all records that come from system headers.
+    if (Kind != SrcMgr::C_User)
----------------
Do you get reports from system headers without this check?


================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:209
@@ +208,3 @@
+      std::tie(Info.Size, std::ignore) =
+          ASTContext.getTypeInfoInChars(FD->getType());
+
----------------
Are you intentionally not using getTypeInfoDataSizeInChars? Can this lead to false positives? A comment would be helpful.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:325
@@ +324,3 @@
+
+    Os << " (" << BaselinePad.getQuantity() << " padding bytes, where "
+       << TargetPad.getQuantity() << " is optimal)";
----------------
Why are you not testing for the full message in the tests? It is important to ensure that the information provided here does not regress.


http://reviews.llvm.org/D14779





More information about the cfe-commits mailing list