User Details
- User Since
- Dec 6 2016, 10:52 AM (355 w, 6 d)
Jul 6 2023
Earlier (IIRC in March) we did an internal test of the check and the following results were obtained. The results are kinda weird, to say the least. Numbers are after CodeChecker has done a serverside uniqueing (most likely things such as "if X in H.h is reported in the compilation/analysis of A.cpp and B.cpp then store X only once") of the bugs.
Mar 23 2023
Feb 17 2023
@vabridgers Please take a look at this, as per off-list discussion, in relation to D142822.
Feb 15 2023
Feb 2 2023
Ping @dyung, it looks like you're the owner of the relevant build-bot. I can't find any information what x86_64-sie is...
Alright, right now, I have no meaningful idea why this failure appears, locally I'm trying every test command as they appear in the test, and all the tests are passing. It's as if on that system the whole Annex K support would not be allowed. Locally I added a few debug prints and I'm getting the right answers for "Whether Annex K is allowed?".
Oddly enough, one of the buildbots caught a not matching test that was working locally... on it.
Alright, I've done a full reanalysis after a rebase. Overhead is not meaningfully measurable, even for complex TUs such as LLVM. The check is viable in C++ code as it finds cases (such as the one described in LLVM, but also Bitcoin is a primarily C++ project), so I won't rework the check to disable it in C++ mode explicitly. It seems the name lookup is implemented pretty well, helped by the fact that the names to match are short. No crashes had been observed in the test projects; let's hope it stays the same way; the matchers themselves are simple enough. The Annex K. matcher is only registered if in >= C11 mode.
I've also gone through the discussion earlier, and it looks to me as if all the concerns were either resolved or made obsolete due to the evolution of the check.
Jan 31 2023
Alright, the tests came back from the CI positively and there were no crashes on like 15 projects (about 50:50 C and C++). There are some nice results:
Jan 30 2023
Tentative LGTM, will still need to run the "usual test projects" in the CI for confirmation. What's missing from this patch are the .rst files for the CERT-specific aliases of the check, but those are trivial and we can "add them in post" anyway.
Oct 21 2022
Oct 17 2022
Oct 15 2022
Sep 27 2022
I agree with @martong that $ = realloc($, ...) is indicative of something going wrong, even if the developer has saved the original value somewhere. Are we trying to catch this with Tidy specifically for this reason (instead of CSA's stdlib checker, or some taint-related mechanism?). However, @steakhal has some merit in saying that developers might prioritise the original over the copy, even if they made a copy. I think an easy solution from a documentation perspective is to have a test for this at the bottom of the test file. And document that we cannot (for one reason or the other) catch this supposed solution, but if you have made a copy already(!), then you might as well could have done void* q = realloc(p, ...) anyway! Having this documented leaves at least some paths open for us to fix false positives later, if they become a tremendous problem. (I have a gut feeling that they will not, that much.)
Sep 5 2022
Generally LGTM. Please revisit the documentation and let's fix the style, and then we can land.
Aug 30 2022
Sorry, I'm not sure how I missed it. Been on vacations and such... either way, I think documenting this is a good practice.
Aug 9 2022
I got here from D131319, but I am not sure why this change is only touching comments but not the hand-written documentation. Is this CastInfo an internal detail that should not be publicly documented to the everyday documentation reader? Are the old-style "classof" method and enums still applicable?
Aug 1 2022
Alright, I think we should have this in and let C++17 things to be future work. Please see the inline comment, but otherwise this should be good enough. Can always improve in a future version. 😉
Jul 21 2022
Jul 14 2022
Jun 22 2022
LGTM. Thanks!
Jun 20 2022
(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!
Jun 19 2022
Thank you!
Jun 14 2022
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
D'oh! I made a mistake when copying the URL and accidentally associated the commit with D12306 instead of D123065... Anyhow, this was committed in rGb1f1688e90b22dedc829f5abc9a912f18c034fbc.
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[8]; 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?