- User Since
- Mar 14 2013, 3:16 PM (223 w, 2 d)
Thu, Jun 22
Aside from a few small nits with the test cases, this LGTM! You should hold off on committing for a day or two in case Richard or others have comments on the patch.
Wed, Jun 21
Fri, Jun 16
A few more nits, but this feels like it's getting close to ready (at least, to me).
This is generally looking good to me, with a few small nits. @rsmith, do you have thought on this?
Thu, Jun 15
Wed, Jun 14
Committed in r305432. You should consider reaching out to Chris Lattner for svn access!
Tue, Jun 13
Adding Richard to the review for some wider perspective than just mine on the overall design.
Thu, Jun 8
Aside from one minor nit, LGTM!
Wed, Jun 7
Aside from one minor nit, LGTM
Aside from a minor suggestion, LGTM!
Mon, Jun 5
This check is an interesting one. The rules around what is signal safe are changing for C++17 to be a bit more lenient than what the rules are for C++14. CERT's rule is written against C++14, and so the current behavior matches the rule wording. However, the *intent* of the rule is to ensure that only signal-safe functionality is used from a signal handler, and so from that perspective, I can imagine a user compiling for C++17 to want the relaxed rules to still comply with CERT's wording. What do you think?
Thank you for working on this. How does this check compare with the -Wcast-align diagnostic in the Clang frontend? I believe that warning already covers these cases, so I'm wondering what extra value is added with this check?
Fri, Jun 2
Thu, Jun 1
Can you help me to understand what problem is being solved with this new attribute? Under what circumstances would the first argument be an ImplicitParamDecl but not an implicit this or self?
Tue, May 30
Addressing review comments.
Can you add some unit tests?
Mon, May 29
I'm not opposed to this check, but I'm not keen on having a check that directly contradicts the output from another check. The cert-dcl21-cpp check will diagnose user's code when a postfix operator ++ or -- is *not* const-qualified. Would it make sense to silence this diagnostic in the presence of also checking for cert-dcl21-cpp for such operators?
May 25 2017
Commit in r303913
Commit in r303912
Once you fix the typo in the check, can you run it over some large C++ code bases to see if it finds any results?
Now, with CMake warning. This is only needed for the llvm cmake files (since Clang relies on them anyway).
May 24 2017
May 23 2017
May 22 2017
May 21 2017
May 20 2017
As an FYI, there is a related check being implemented in clang currently; we probably should not duplicate this effort. See https://reviews.llvm.org/D33333.
As an FYI, there is a related check currently under development in clang-tidy; we probably should not duplicate this functionality in both places. See https://reviews.llvm.org/D19201 for the other review.
May 18 2017
May 12 2017
May 11 2017
Please be sure to regenerate the AST matcher documentation as well by running clang/docs/tools/dump_ast_matchers.py
May 10 2017
This looks reasonable to me.
May 9 2017
Have you tried running this over a large code base to see how frequently the diagnostic triggers? I'm wondering why 10 was chosen as the default threshold.
May 8 2017
Committed in r302419, thank you!
LGTM, thank you! Do you need me to commit this for you?
May 5 2017
Thanks! I've commit in r302275.
May 3 2017
Thank you for working on this check!