- User Since
- Mar 14 2013, 3:16 PM (322 w, 4 d)
LGTM aside from some nits, thank you for this!
If you wanted to expand test coverage for the changes covered in this, that would be a good thing. However, I'm not certain how testable the newly colorized output is, which is why this LG as-is.
LGTM (with the typo fix).
Fri, May 17
Thu, May 16
LGTM aside from some commenting requests.
Thanks for this -- it looks like really interesting functionality! I've mostly found nits thus far, but did have a question about clang-query support for it.
What will be making use of/testing this new functionality?
I'm not certain where you're planning to go with this change (or is this the only change you're trying to make in this area?), so it's a bit hard to evaluate this patch. Can you explain a bit more about what you're ultimately trying to accomplish?
Tue, May 14
I've commit in r360667, thanks!
Mon, May 13
Switched to using the JSON streaming interface rather than a custom solution
Updated the test cases for the new formatting
Adding some more AST matcher reviewers to see if there are other options that we've not considered.
Fri, May 10
LGTM with some testing nits.
Thu, May 9
Wed, May 8
This LGTM, but you may want to wait a day or so to see if @alexfh has any remaining concerns.
Aside from a formatting issue, this LGTM, thank you!
Tue, May 7
Thank you for updating the test! One minor nit with the way the test case is written, but otherwise LGTM.
LGTM modulo the comments from @gribozavr.
Updating based on review feedback.
I've commit in r360147, thank you for the patch!
I found a few nits, but generally think this LG. However, I think @rsmith should sign off on it just in case I've misinterpreted something along the way.
Mon, May 6
Sun, May 5
You should also regenerate the documentation by running clang/docs/tools/dump_ast_matchers.py.
Fri, May 3
You should also update Registry.cpp to add this to clang-query and you should also regenerate the documentation by running clang\docs\tools\dump_ast_matchers.py.
I would still like to see the second RUN line added to the test so we can ensure we don't regress the behavior.
Thu, May 2
Out of curiosity, have you run this over any large code bases to see what the false positive and true positive rate is?
Wed, May 1
LGTM, thank you!
LGTM, thank you! That's strange that clang-format would remove the newline from the end of the file. I don't get that behavior when I try it, so it makes me wonder what's different.