- User Since
- Dec 6 2016, 10:52 AM (291 w, 19 h)
Wed, Jun 22
Mon, Jun 20
(Please ensure a more appropriate commit message that actually mentions the check when committing, @steakhal.)
Because of the stability issues related to getName()-like constructs I'm putting a temporary ❌ on this (so it doesn't show up as faux accept). However, I have to emphasise that I do like the idea of the check!
Sun, Jun 19
Tue, Jun 14
The context of the diff is missing. Please re-run the diff making with -U9999999999999999999.
May 22 2022
May 18 2022
May 17 2022
May 16 2022
May 13 2022
- Added to ASTMatchers/Registry.cpp
- Updated with release notes
May 12 2022
(Also: this is a fix to an issue of your own finding and not something that was reported on Bugzilla? Just to make sure we cross-reference and close tickets.)
LGTM. Some typos inline. Also I think this is a significant enough bug-fix that it warrants an entry in the Release Notes under Changes to existing checks.
May 11 2022
@njames93 What do you think about the current approach? It will under-approximate the problem-inducing node set but at least cover what we know about C++ now.
Just one question if you could try this out for me: what happens if you run clang-tidy a.c b.c (two TUs in the invocation) where one of them (preferably the later one, i.e. b.c) does NOT have Annex K enabled? I believe the cached IsAnnexKAvailable (like any other TU-specific state of the check instance) should be invalidated/cleared in an overridden void onStartTranslationUnit() function.
May 5 2022
Apr 30 2022
This is unrelated to Clang-Tidy, the change affects the LLVM ADT library.
Apr 28 2022
Apr 27 2022
Apr 26 2022
Regarding FixIts... FixIts are implemented in the "Diagnostic" library, which is non-specific to neither Clang-Tidy nor Sema whatsoever, they use the same infrastructure under the hood. Why the apply logic in CSA might do the same FixIt multiple times is beyond me, but I know that both clang-apply-replacements and clang-tidy go to length to ensure that in case multiple checkers report to the same location with potentially conflicting FixIts, then none gets applied, because applying all of them would result in ridiculously broken source code. They internally become an object in the clang::tooling namespace which is implemented as a core Clang library. The relevant entrypoint to this logic, at least in Clang-Tidy, should be this one: http://github.com/llvm/llvm-project/blob/8f9dd5e608c0ac201ab682ccc89ac3be2dfd0d29/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L115-L134
After adding improvements to the documentation, I think this will be good to go, and thank you! Perhaps just for a safety measure you could run it on a few projects (LLVM itself?) to ensure we didn't miss a case where it might magically crash, but I wonder how many specifically "C++14" projects will use signal handlers in the first place.
Apr 20 2022
Apr 19 2022
[NFC] Fix the test.
@aaron.ballman Alright, I think this can go. The ReleaseNotes.rst needs a rebase anyway.
Mar 30 2022
(Side note: you should avoid the list-expansion syntax in URLs because browsers do not understand them and result in links that are not leading anywhere.)
Mar 29 2022
Alright, I think this is good to go. I like that it makes it clear that the called function is coming from some external source (system header or otherwise).
Mar 17 2022
Mar 10 2022
Mar 9 2022
I have some concerns about this. While it is now clear to me that the partialness refers to partial coverage of the guideline rule, it is indeed very, very partial. MEM51-CPP as of now lists 9 cases of non-compliant examples, from which std::shared_ptr<T> = new S; is just one. bugprone-shared-ptr-array-mismatch (D117306) in itself is a check that inherits from a "more generic" smart pointer check.
Mar 8 2022
Is "partial aliasing" a new notion? Or... is the alias not partial, but the coverage?
Mar 1 2022
This generally looks good, and thank you! I have a few minor comments (mostly presentation and documentation).
Feb 28 2022
Feb 25 2022
Thanks! I'll re-run the tests on top of 14.0 and do the backport too, soon. 🙂
Added Release notes entry.
Jan 24 2022
Nov 30 2021
Should/does this work in C++ mode for std::whatever?
Nov 26 2021
I haven't looked at the patch in detail yet, but I know I've run into the issue this aims to fix when developing a Tidy check!
hasParent() is the direct upwards traversal, right? I remember some discussion about matchers like that being potentially costly. But I can't find any description of this, so I guess it's fine, rewriting the matcher to have the opposite logic (decl(hasDescendant(...the-real-matcher-here...)).bind("blah")) would just make everything ugly for no tangible benefit.
(@carlosgalvezp Removing you from reviewers so the patch shows up as not yet reviewed for others! The significant part is the Sema which should be reviewed.)
I believe this is worth a note in the ReleaseNotes.rst file. People who might have disabled the check specifically for the reason outlined in the PR would get to know it's safe to reenable it.
This generally looks fine for me, but I'd rather have other people who are more knowledgeable about the standard's intricacies looked at it too.
Thank you, this is getting much better!
I'm not sure if it is a good idea to merge things on a Friday, but I am comfortable with this, let's get the check crash less.
Nov 22 2021
Nov 18 2021
As a user, I'm not sure if this would be the right way moving forward. This patch conflates a lot of parallel (but related) executions of the idea. (I'm not against introducing points 1 and 2 in the same patch, but do not feel like it's right to do it this way!)