bcraig closed this revision.
bcraig added a comment.
Submitted to r255545
http://reviews.llvm.org/D14779
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
bcraig marked 2 inline comments as done.
bcraig added a comment.
http://reviews.llvm.org/D14779
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
bcraig updated this revision to Diff 42742.
bcraig marked an inline comment as done.
http://reviews.llvm.org/D14779
Files:
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
test/Analysis/padding_c.c
test/Ana
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/Checkers.td:49
@@ -48,1 +48,3 @@
+def Performance : Package<"performance">;
+
zaks.anna wrote:
> I think Performance should be in the OptIn package.
What do you think about this one?
http
bcraig marked 3 inline comments as done.
bcraig added a comment.
In http://reviews.llvm.org/D14779#309170, @zaks.anna wrote:
> With respect to the issues this checker found, I suggest submitting patches
> for the clang issues that can be fixed. Can the x-macro case be suppressed
> with the reco
zaks.anna added a comment.
With respect to the issues this checker found, I suggest submitting patches for
the clang issues that can be fixed. Can the x-macro case be suppressed with the
recommended suppression? I'd also submit a patch to gtest. Submitting patches
with the fixes provides a good
bcraig updated this revision to Diff 42524.
bcraig marked 3 inline comments as done.
bcraig added a comment.
Anna Zaks last round of review requests.
http://reviews.llvm.org/D14779
Files:
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer
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 wrot
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 cal
bcraig updated the summary for this revision.
bcraig updated this revision to Diff 40818.
bcraig added a comment.
Adding a test to validate non-trivial message components.
Adding recommendations to the diagnostic on how to fix and / or suppress the
generated report.
Changing default padding allow
bcraig added a comment.
In http://reviews.llvm.org/D14779#293339, @zaks.anna wrote:
> > can be suppressed without breaking ABI by adding explicit padding members.
>
>
> We should convey the suppression mechanism as part of the diagnostic.
I can make the diagnostic longer. Alternatively, is the
zaks.anna added a comment.
> can be suppressed without breaking ABI by adding explicit padding members.
We should convey the suppression mechanism as part of the diagnostic.
http://reviews.llvm.org/D14779
___
cfe-commits mailing list
cfe-commits@l
bcraig marked an inline comment as done.
bcraig added a comment.
In http://reviews.llvm.org/D14779#293236, @zaks.anna wrote:
> > I have seen plenty of structures where the specific layout was important
> > and couldn't be changed.
>
>
> Can you give specific examples of these? Can we develop heu
zaks.anna added a comment.
> I have seen plenty of structures where the specific layout was important and
> couldn't be changed.
Can you give specific examples of these? Can we develop heuristics for them?
> These generally felt like noisy reports unless I had more specific
> justification fo
bcraig marked 15 inline comments as done.
bcraig added a comment.
http://reviews.llvm.org/D14779
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
bcraig updated this revision to Diff 40659.
bcraig added a comment.
Addressed the bulk of Anna's review comments.
http://reviews.llvm.org/D14779
Files:
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
test/
bcraig added a comment.
In http://reviews.llvm.org/D14779#292513, @zaks.anna wrote:
> 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?
I ran this over a large C code base, then
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
bcraig added a comment.
In http://reviews.llvm.org/D14779#292066, @zaks.anna wrote:
> > I may be mistaken, but this check looks more appropriate for Clang-tidy.
>
>
> This is a syntactic check. Both clang-tidy as well as the clang static
> analyzer contain this type of checks. If we move all syn
zaks.anna added a comment.
> I may be mistaken, but this check looks more appropriate for Clang-tidy.
This is a syntactic check. Both clang-tidy as well as the clang static analyzer
contain this type of checks. If we move all syntactic checks to clang-tidy, the
users that use the analyzer but
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.
I may be mistaken, but this check looks more appropriate for Clang-tidy.
http://reviews.llvm.org/D14779
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http:/
21 matches
Mail list logo
