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

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

> An analysis of llvm+clang+compiler-rt now only generates 16 excessive padding reports in index.html.


Should those be fixed or are they all false positives?


================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:48
@@ +47,3 @@
+    // The calls to checkAST* from AnalysisConsumer don't end
+    // visit template instantiations or lambda classes.  We
+    // want to visit those, so make our own RecursiveASTVisitor.
----------------
Wording is off here.
Would it be possible to provide a checkAST method that does visit temple instantiations and lambdas? What if another checker wants the same functionality?

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:70
@@ +69,3 @@
+
+  void visitRecord(const RecordDecl *RD, uint64_t PadMultiplier = 1) const {
+    if (shouldSkipDecl(RD))
----------------
Could you add a comment explaining what is PadMultiplier and why it is needed?

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:196
@@ +195,3 @@
+    auto LeftFoldFunc =
+        [&ASTContext, &RL](const CharUnitInfo &Old, const CharUnitInfo &New) {
+          CharUnitInfo Retval;
----------------
Is this doing more than just accumulating the padding for every field in the  record? For example, it seems we could do it with a simple loop the does:
  PadSum += Current.Offset - (Prev.Offset + Prev.Size)

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:213
@@ +212,3 @@
+
+  static CharUnits calculateOptimalPad(const RecordDecl *RD,
+                                       const ASTContext &ASTContext,
----------------
Please, add a high-level description of how optimal pad is calculated.


================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:223
@@ +222,3 @@
+        // In the typical case, this will result in us popping
+        // the back element of the vector, instead of shifting lots
+        // of elements.
----------------
The second comment is unclear. Which vector are you talking about here?

I think it would be better to just move the comments to the places where the action is performed; ex std::sort(Fields.begin(), Fields.end()) in this case.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:230
@@ +229,3 @@
+    SmallVector<CharUnitPair, 20> Fields;
+    auto Transformation = [](const FieldDecl *FD) {
+      CharUnitPair RetVal;
----------------
Please, pick a more descriptive name.

================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:249
@@ +248,3 @@
+    CharUnits NewOffset = ASTContext.toCharUnitsFromBits(RL.getFieldOffset(0));
+    CharUnits NewPad;
+
----------------
Is NewPad initialized?


http://reviews.llvm.org/D14779