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