[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

steakhal added a comment. Thank you guys the responses. I cannot agree more. The sole reason why this option exists is that if you scroll back in the git blame of that line, you would find a commit, which removed this warning for this exact scenario. Possibly due to some seemingly false positive

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

Szelethus added a comment. In D66733#1706521 , @sylvestre.ledru wrote: > I added it to the release notes here : https://reviews.llvm.org/rC374593 > I am wondering if the option( WarnForDeadNestedAssignments ) to disable it > is really necessary? > I ha

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

sylvestre.ledru added a comment. It now shows for the llvm toolchain: http://llvm.org/reports/scan-build/ Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66733/new/ https://reviews.llvm.org/D66733 ___ cfe-commits mailin

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

sylvestre.ledru added a comment. I added it to the release notes here : https://reviews.llvm.org/rC374593 I am wondering if the option( WarnForDeadNestedAssignments ) to disable it is really necessary? I haven't seen any false positive while deadstore has some. Repository: rL LLVM CHANGES SI

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

sylvestre.ledru added a comment. @steakhal Thanks for this patch. I tried it on Firefox and it found a bunch of stuff we improved. Some examples: https://hg.mozilla.org/integration/autoland/rev/db24db8f5f549ff446d1c3c69799187bcc2409e2 https://hg.mozilla.org/integration/autoland/rev/5de53dab979a40

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

NoQ added inline comments. Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:133 std::unique_ptr> InEH; + const bool WarnForDeadNestedAssignments; Szelethus wrote: > Szelethus wrote: > > NoQ wrote: > > > I suggest we adopt the idiom of

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

Szelethus added inline comments. Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:133 std::unique_ptr> InEH; + const bool WarnForDeadNestedAssignments; Szelethus wrote: > NoQ wrote: > > I suggest we adopt the idiom of passing the `Che

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

Szelethus added inline comments. Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:133 std::unique_ptr> InEH; + const bool WarnForDeadNestedAssignments; NoQ wrote: > I suggest we adopt the idiom of passing the `Checker` object around a

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

NoQ added inline comments. Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:133 std::unique_ptr> InEH; + const bool WarnForDeadNestedAssignments; I suggest we adopt the idiom of passing the `Checker` object around and asking it about

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

This revision was automatically updated to reflect the committed changes. Closed by commit rL370767: [analyzer] Add a checker option to detect nested dead stores (authored by Szelethus, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

Szelethus accepted this revision. Szelethus added a comment. Excellent detective work! Thanks! I'll do the honors. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66733/new/ https://reviews.llvm.org/D66733 ___ cfe-commits mailing list cfe-com

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

steakhal updated this revision to Diff 217665. steakhal added a comment. Changes: - Flag option marked as 'enabled by default'. - Reformat all the test cases for C, C++ and Obj C. - Now uses `-verify=tags` approach. - Fixes checker documentation. CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

steakhal marked 2 inline comments as done. steakhal added a comment. Fixes for @NoQ's comments. I will update the patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66733/new/ https://reviews.llvm.org/D66733 ___ cfe-commits mailing list c

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

Szelethus added a comment. In D66733#1647964 , @steakhal wrote: > @Szelethus The mispositioned report message was my fault. I used a different > version of clang for the analysis and to upload the results, which resulted > in some mispositioned reports.

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

NoQ added a comment. Mmm, i don't know what was that commit meant to fix. Your evaluation looks fairly sufficient for turning it on by default. Let's make it an on-by-default option and flip the flag back into off-by-default if it turns out to be somehow broken. Comment at:

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

steakhal added a comment. @Szelethus The mispositioned report message was my fault. I used a different version of clang for the analysis and to upload the results, which resulted in some mispositioned reports. I've fixed the linked CodeChecker instance. CHANGES SINCE LAST ACTION https://revi

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

steakhal added a comment. @Szelethus Your catch with the mispositioned report message

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

steakhal added a comment. @NoQ What do you think, should it be under a flag (as it would be now), or enabled by default? I think these warnings are valuable and we should consider it enabling by default. An interesting fact is that previously rGf224820b45c6847b91071da8d7ade59f373b96f3

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

steakhal updated this revision to Diff 217164. steakhal added a comment. Fix copy-paste mistake. This time upload the correct version. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66733/new/ https://reviews.llvm.org/D66733 Files: clang/docs/analyzer/checkers.rst clang/include/clan

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

steakhal updated this revision to Diff 217162. steakhal added a comment. Reformatted using `clang-format-diff.py`. Minor fixes which were requested by @Szelethus. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66733/new/ https://reviews.llvm.org/D66733 Files: clang/docs/analyzer/check

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

steakhal marked 3 inline comments as done. steakhal added a comment. Thank you for your response @Szelethus. Fixed, updating the patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66733/new/ https://reviews.llvm.org/D66733

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. I really-really like this change. The results look great, though I'm not sure what happened here

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

steakhal created this revision. steakhal added reviewers: NoQ, krememek, Szelethus, baloghadamsoftware. Herald added subscribers: cfe-commits, Charusso, donat.nagy, dexonsmith, mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun, whisperity. Herald added a project: clang. Enables the users to