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