- User Since
- Jul 10 2012, 10:35 AM (371 w, 3 d)
Ideally, it would be nice to sort the list of registrations by the check name. But given that the call can span two lines, that may be slightly trickier to implement. It's worth giving it a try though ;)
Tue, Aug 20
Fri, Aug 16
Just FYI, https://bugs.llvm.org/show_bug.cgi?id=43019 is relevant.
Wed, Aug 14
Tue, Aug 13
Mon, Aug 12
One more nit.
Wed, Aug 7
Fri, Jul 26
A general comment: "misc" is a sort of a heap of checks that otherwise don't have a good home. This one would probably better go to bugprone (or maybe there's a relevant CERT or C++ Core Guidelines rule?).
I think it will be a strict improvement to include the check name into the deduplication key (probably after the file and offset and before the message). I don't see any reason to hide this behind a flag or otherwise retain the old behavior. As for expanding the key to include notes and fixes - it's probably good to do this either, and this may help uncover incorrect behavior of some checks. I'd suggest to start with the check name though.
Jul 22 2019
LessClangTidyError only compares location and message, but it could also compare other things like notes, fixes, etc. For the problem outlined in the description of this patch we can probably include the checker name into the key. WDYT?
Jul 18 2019
Jul 17 2019
Seems good now. Haojian, do you have any concerns?
Jul 16 2019
Jul 15 2019
Jul 1 2019
May 20 2019
That's a quite impressive amount of work. Thanks! Looks good.
May 14 2019
May 9 2019
May 8 2019
LG. Thank you!
May 7 2019
May 6 2019
May 3 2019
Apart from NOLINT handling there's more logic in ClangTidyDiagnosticConsumer::HandleDiagnostic, which isn't properly transferred to ClangTidyContext::diag in this patch. The logic that is transferred seems to change the behavior w.r.t. notes that can "unmute" ignored warnings (see https://reviews.llvm.org/D59135#1456108). I suspect that we're missing proper test coverage here. Another issue is that compiler diagnostics don't pass ClangTidyContext::diag in the non-plugin use case. Do all the existing tests pass with your patch?
Please regenerate the HTML docs using clang/docs/tools/dump_ast_matchers.py.
Apr 24 2019
LG. Thanks for the fix!
Apr 23 2019
LG with a comment.
Apr 18 2019
There's one more nit. Otherwise good to go.
Apr 17 2019
LG with a couple of nits.
A few post-commit comments.
Artem, I'd appreciate, if you found time to finish this fix. The bug is causing most failures in our setup. Thanks!
Apr 16 2019
Apr 15 2019
Thanks! The change looks good now.
Apr 10 2019
Thanks for the useful check! I have a few comments, see inline.
Apr 8 2019
It looks like there's a number of users of this function beyond what you've mentioned:
Apr 5 2019
Looks like this check would fit better into the bugprone module.
Can you give a specific example of how this problem manifests?
Thanks, looks better now, but still a few comments, mostly nits.
Apr 4 2019
Apr 3 2019
Thanks for addressing this! Please add the test cases from https://bugs.llvm.org/show_bug.cgi?id=41185
This looks like a more promising direction. Thanks for the readiness to experiment with this.