- User Since
- Feb 7 2019, 5:16 AM (90 w, 1 d)
Aug 29 2020
Note texts are now describing what kind of C++ constructs were used. The warning message also explains better the reason of the issue: "signal handlers must be plain old functions, C++-only constructs are not allowed"
Test file has been extended with the second line of the "note" diagnostic message which designates the location of the suspicious "C++ construct". The full clang-tidy output looks like this:
Aug 28 2020
I rebased the patch so it compiles with master version of LLVM/Clang. I did no other change, so I would like if this patch would be committed on behalf of @NorenaLeonetti if the patch is acceptable.
I would kindly ask the reviewers to give some comments if any additional modification is needed. I run this checker on DuckDB project and this checker gave two reports on functions which shouldn't be used as signal handler:
I'll rebase this patch and continue its development.
Aug 26 2020
I reverted some changes so this checker remains an alpha checker. This patch now contains only the elimination of the false positive case. I wonder if this change is acceptable.
Jul 16 2020
Jul 13 2020
Jun 24 2020
Thanks everyone for the investigation in this problem.
I find these suggestions reasonable, and I think this commented example is a strong beating card in our hands to convince our users that paths going through system headers are meaningful and important.
I'll start implementing this idea and we'll see how much it affects the results in real-world projects.
May 21 2020
@baloghadamsoftware You're right, but I couldn't find a header file which (1) contains a bug which ClangSA reports, (2) is not an "expected-warning", (3) is inside an already existing directory so it could be considered a system header.
May 20 2020
@balazske Alright, I rephrased it according to your suggestion.
May 19 2020
May 13 2020
Sep 6 2019
Usually I'm not a big fan of auto, but in this case it makes sense not to duplicate the type name in the same expression. I'll use this rule of thumb in the future, thanks :)
Thank you for the suggestion. I didn't know about this behavior of %check_clang_tidy.
Aug 17 2019
The checker message has been changed according to the suggestion.
The last parameter of checkNonNull() doesn't require a default or optional value, so it has been fixed.
The parameter has been renamed from Idx to IdxOfArg.
Aug 16 2019
Thank you for the valuable comments and the review!
I'm just planning to register to the bug tracker, because it is getting more relevant due to some other contributions as well.
Aug 14 2019
Since now the order of diagnostic messages is deterministic, we can count on this order in the test file.
Aug 6 2019
This suggestion would result another strange behavior: if the user disables cert-err09-cpp because he or she doesn't want to see its reports, the other one (cert-err61-cpp) will still report the issue. So he or she has to disable both (or as many aliases it has).
Not exactly. The problem is that it is non-deterministic which checker reports the issue. For example before this patch, in the test file above there was only one report. However, sometimes the report message is:
Aug 5 2019
Alright, I modified the commit accordingly. Thank you for the suggestions.
Jul 23 2019
Yes, LessClangTidyError would really be the best place for this. The reason of my implementation was that I wanted to bind this feature to a command line option. I don't know if there was any design decision behind the current uniquing logic and I didn't want to break it if there was any. So would you be agree with having duplicate reports by default without any command line flag in case of alias checkers? I believe this would be the most painless solution, since in case of further improvements about including notes, fixes, etc. in the functor, the potential command line option controlling this would be unnecessarily complex. I don't know if there could be any use-case on those.
Thanks for the review!
Jul 22 2019
Mar 14 2019
I've uploaded another version of the last fix. The previous one contained an UB, although it worked practically.
Mar 13 2019
I added a condition before std::next() invocations to check if the next element is inside the valid interval. This fixes the crash of the build-bot. Sorry for the ugly bug.
I don't know if there is a more elegant solution.
Mar 12 2019
I rebased the patch on the current master.
Mar 7 2019
I added a test case for this recursive case. There is also a TODO in the code indicating the place where an additional fix will be required.
Feb 10 2019
Feb 8 2019
There was another place where this crash could have happened.
I've added a test case.