User Details
- User Since
- Mar 14 2013, 3:16 PM (266 w, 3 d)
Thu, Apr 19
Tue, Apr 17
Have you run this over any large code bases to see whether the check triggers in practice?
Committed in r330188, thank you for the patch!
LGTM! I will go ahead and commit on your behalf. Btw, if you're thinking of making additional patches, now might be a good time to ask for commit privileges (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access).
I've committed in r330185, thank you for the patch!
Mon, Apr 16
Committed in r330159. Thank you for the patch!
LGTM!
Sat, Apr 14
LGTM aside from a few small nits.
Fri, Apr 13
I'd also be interested to see the number of false positives and true positives when run over some large code bases (both C and C++). For instance, I would imagine code like this to be somewhat common and very reasonable:
void some_func(int someArg, bool *someResult) { if (someResult) *someResult = false; }
Especially in C code where references are not available. This makes me wary of making this a compiler diagnostic, but clang-tidy may still be a reasonable place for this functionality to live.
Thu, Apr 12
Committed the fix in r329930.
I think this code is ready to go in -- accepting my own revision so that I can close. If @delesley spots any issues, they can be address post commit.
I've commit in r329904, thank you for the patch!
Wed, Apr 11
Aside from some minor nits, LGTM!
Tue, Apr 10
I see we all found the solution at the same time. Yay teamwork! :-D
Thank you for rebasing, but the patch does not pass tests locally for me. I get the following results (testing on Windows x64 with MSVC 2017 in a Debug build):
Thank you for working on this!
LGTM!
Can you rebase the patch? The patch did not apply cleanly for me against trunk (the contents of the check function are appearing in CheckARMBuiltinExclusiveCall()).
Ping.
Ping.
LG aside from a diagnostic wording nit.
LGTM with some minor commenting nits.
LGTM aside from a minor docs nit.
Mon, Apr 9
I don't think we're currently diagnosing static data members of classes as being unused in the first place; are there plans to implement that functionality so that someone might want to write the attribute there?
LGTM with a few minor nits to be fixed.
Sorry for the delayed review; I'm back from vacation now and am picking things up again.
Mar 22 2018
Adding some folks to review the dynamic matcher bits.
Mar 21 2018
LGTM with a minor doc nit.
LGTM!
This generally looks reasonable to me, but @rsmith should weigh in before you commit because MultiSourceLocation is novel.
I think hasSourceExpression() already does what you're looking for.
void f() { int i = 1.0f; }
clang-query> match implicitCastExpr(hasSourceExpression(floatLiteral()))
Mar 20 2018
You should also regenerate the AST matcher documentation by running docs/tools/dump_ast_matchers.py
LGTM!
I don't know enough about ObjC to feel comfortable reviewing this, but I've added @rjmccall who may have opinions.
Mar 19 2018
Now: with more context!
Ping.
Mar 17 2018
This looks reasonable to me, but I'm also not a python expert.
Mar 16 2018
Mar 14 2018
LGTM with some minor documentation nits.
LGTM!
Adding Richard as a reviewer, because this seems somewhat clever to me (so there may be a better change to make).
LGTM aside from a testing nit.
LGTM, thank you for the explanation about the tests.
Test case for this change?
Mar 13 2018
I dreamed up some test cases that may or may not make sense. I think they boil down to a few things:
Mar 12 2018
LGTM with the test comments fixed up.