Re: [PATCH] D23387: [Analyzer] Report found fields order in PaddingChecker.

compnerd closed this revision. compnerd added a comment. SVN r278730 https://reviews.llvm.org/D23387 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23387: [Analyzer] Report found fields order in PaddingChecker.

alexshap added a comment. i don't have commit access, if you could land this patch i would be grateful https://reviews.llvm.org/D23387 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23387: [Analyzer] Report found fields order in PaddingChecker.

bcraig added a comment. LGTM Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:217 @@ +216,3 @@ +// then large field indices to small field indices +return std::make_tuple(Align, -Size, + Field ? -static_cast(Field->getFiel

Re: [PATCH] D23387: [Analyzer] Report found fields order in PaddingChecker.

alexshap marked 2 inline comments as done. alexshap added a comment. ping https://reviews.llvm.org/D23387 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23387: [Analyzer] Report found fields order in PaddingChecker.

alexshap added inline comments. Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:217 @@ +216,3 @@ +// then large field indices to small field indices +return std::make_tuple(Align, -Size, + Field ? -static_cast(Field->getFie

Re: [PATCH] D23387: [Analyzer] Report found fields order in PaddingChecker.

bcraig added a comment. LGTM. Thanks for the patch! Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:217 @@ +216,3 @@ +// then large field indices to small field indices +return std::make_tuple(Align, -Size, + Field ? -st

Re: [PATCH] D23387: [Analyzer] Report found fields order in PaddingChecker.

alexshap updated this revision to Diff 67797. alexshap added a comment. Update the tests. Update the format again (do not include the type name because the diagnostics becomes too verbose and unreadable (i ran it against LLVM and clang - with type names the diagnostic messages are getting very l

Re: [PATCH] D23387: [Analyzer] Report found fields order in PaddingChecker.

alexshap added a comment. toy example: F2262237: toy_example.png https://reviews.llvm.org/D23387 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23387: [Analyzer] Report found fields order in PaddingChecker.

alexshap updated this revision to Diff 67773. alexshap added a comment. Use more stable sort (preserving the original order if possible). Change the format of the report. Update the tests. I have not turned it to "note:" or "remark:" because in this case it's not included into the description and

Re: [PATCH] D23387: [Analyzer] Report found fields order in PaddingChecker.

alexshap added a comment. Thanks, your points regarding better formatting of the diagnostics / moving it into a note or fixit and using more stable sort are valid, i will update this diff today or tomorrow. Regarding the type name - i am not sure - for example for template specializations (for

Re: [PATCH] D23387: [Analyzer] Report found fields order in PaddingChecker.

bcraig added a comment. I am kind of interested in what the warning message for clang::Sema in clang/include/clang/Sema/Sema.h looks like. When I last analyzed that file, there was a bunch of padding. It's a very long class though, so it can stress the usability of your messages. https://re

Re: [PATCH] D23387: [Analyzer] Report found fields order in PaddingChecker.

bcraig added a comment. Note: In the future, try to provide more context lines in the patch. http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface I'm mostly fine with the algorithmic changes. I'm not thrilled with the formatting of the error message, but I'm not sur