bcraig marked 7 inline comments as done. bcraig added a comment. Somehow, I thought I was waiting on Anna, but I missed the Dec 1 update during the Thanksgiving break, and nothing was blocking me. An updated patch should be posted soon.
In http://reviews.llvm.org/D14779#299895, @zaks.anna wrote: > > 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? All of them are real problems. - 6 of them would be straightforward to fix - 9 of them are the exact same (difficult to fix) issue that gets duplicated because the filenames appear different - projects/compiler-rt/lib/**tsan**/../sanitizer_common/sanitizer_flags.h vs. projects/compiler-rt/lib/**asan**/../sanitizer_common/sanitizer_flags.h - It's a difficult fix because it is a structure where the elements are defined via a preprocessor x-macro. - 1 is in gtest-internal-inl.h, and would be straightforward to fix, but I don't know if we would because it might be considered external code. ================ 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. ---------------- zaks.anna wrote: > 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? Fixed the wording. I don't know of a good way to make the Analysis consumer just pass template instantiations and implicit code to only the checkers that want them. The root of a template instantiation or implicit code tree could be pretty straightforward, but once the children of those nodes start to be examined, it becomes a lot more difficult to determine if a particular call to malloc (for example) should be dispatched to a particular checker. I'm not opposed to the idea, but I would expect that kind of change to go in a different patch. ================ Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:196 @@ +195,3 @@ + auto LeftFoldFunc = + [&ASTContext, &RL](const CharUnitInfo &Old, const CharUnitInfo &New) { + CharUnitInfo Retval; ---------------- zaks.anna wrote: > 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) Switched to raw loop. There has been some advice floating around the C++ community for a little while suggesting that developers should avoid raw loops and use STL and STL like algorithms instead. ( https://channel9.msdn.com/Events/GoingNative/2013/Cpp-Seasoning ). To be honest though, the raw loop is a lot easier to read (IMO) than the two lambas + functional programming algorithm. The raw loop is certainly shorter. ================ Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:249 @@ +248,3 @@ + CharUnits NewOffset = ASTContext.toCharUnitsFromBits(RL.getFieldOffset(0)); + CharUnits NewPad; + ---------------- zaks.anna wrote: > Is NewPad initialized? CharUnits has a default constructor that initializes the value to 0. http://reviews.llvm.org/D14779 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
