- User Since
- Sep 17 2012, 3:16 AM (278 w, 1 d)
Thu, Jan 11
- Fixed review comments
Tue, Jan 9
I like this patch, only found a few nits after the other's review.
I am fine with suppressing the escape globally.
I did see some code in the wild where the constructors registered the objects with a (global) map.
But I think it is still easier to annotate code that does something unconventional than the other way around.
(@xazax.hun - this is an alpha checker last touched by you, do you still have plans for it?)
Do you have a plan for the new false negatives when c++-allocator-inlining is on? Should the user mark allocation functions with attributes?
Overall looks good to me, one comment inline. I think it is good to have these checks to prevent the analyzer executing undefined behavior. Maybe this would make it more feasible to run the analyzer with ubsan :)
In the future, it would be great to also look for these cases symbolically, but I believe it is perfectly fine to have that in a separate patch.
Mon, Jan 8
A high level comment: while it is very easy to get all the gotos using grep, it is not so easy to get all the labels. So for this check to be complete, I think it would be useful to also find labels (possibly having a configuration option for that).
Sat, Jan 6
https://reviews.llvm.org/D41538 is superior and committed.
Thu, Jan 4
- Address some review comments.
Wed, Dec 27
- Address review comments.
Fri, Dec 22
In case you do not like this solution, I uploaded an alternative approach: https://reviews.llvm.org/D41538
Thu, Dec 21
Wed, Dec 20
Committed in https://reviews.llvm.org/rL321190
Is it possible that this will hide other problems? Wouldn't it be better to run the tests twice once with this argument and once without it?
Maybe debug.AnalysisOrder could be used to test the callback order explicitly. This way the test could also serve as a documentation for the callback order.
It was reverted due to tests failures on windows build bots.
In the tests there are multiple variants of the strcpy function guarded by macros. Maybe we should run the tests multiple times to test all variants with different defines?
There was two accepts and the comments found by Alex were resolved, so I went ahead committing this. In case there are further comments, we can address them separately.
Tue, Dec 19
- Address some review comments
- Rebased on ToT
Dec 16 2017
I think, while the analyzer is more suitable for certain kinds of checks that require deep analysis, it is still useful to have quicker syntactic checks that can easily identify problems that are the results of typos or incorrectly modified copy and pasted code. I think this check is in that category. Also, the original warning Peter mentioned does something similar but has some shortcomings.
Just to be sure, this is just a refactoring to make this cleaner or you expect this to have other effects as well, like better performance?
The code looks good to me. But the best way to verify these kinds of changes to see how the results change on large projects after applying the patch.
In the future, we might want to model the standard placement new and return a symbol with the correct SpaceRegion (i.e.: the space region of the argument).
Dec 12 2017
- Further improvements to python script part.
Dec 8 2017
Dec 7 2017
- @gerazo addressed some review comments in scan-build-py.
This does not support memberExprs as condition variables right now.
I like the idea of adding those assertions but a bit worried about the other changes. Basically (if I get this right), we are masking the issues here and I am a bit afraid that they will get forgotten. I think it would be nice to at least add a FIXME that this path should never be triggered.
Dec 4 2017
Nov 29 2017
Nov 28 2017
Found one possible problem. Once fixed, LG!
I found some nit, otherwise LG!
Nov 27 2017
Nov 26 2017
Nov 24 2017
@dcoughlin do you have some input on this?
Nov 23 2017
Nov 21 2017
The checker documentation should be updated as well.
Ok, https://reviews.llvm.org/D39886 is an independent fix for the same issue. It might be useful to add the test cases from that patch to this one as Aleksei pointed out.
The current status of this one, the windows build bot failed, but we could not reproduce it (even on windows). Is there someone who could reproduce the fail? Should we just recommit this hoping that the fail was unrelated to this patch?
Other than two nits it looks good to me. Wait one more reviewer just in case.
Nov 17 2017
A nit, otherwise LG!
I found some nits, but overall I think this is getting close.
Nov 16 2017
Nov 15 2017
Nov 14 2017
Do you expect false positives with this check? Are there cases where the user deliberately wants to break a long critical section to read an updated value (possibly by other thread) in the next one?
Just some quick comments I only checked the first few lines.
Nov 13 2017
I agree it might be useful to expose this matcher to everybody.
Doug added anonymous structure handling, added as a reviewer in case he wants to have a look.
Nov 8 2017
Nov 7 2017
- Fix doc comments that I overlooked earlier
- Fix review comments
LGTM! But wait one more reviewer to be sure :)
This looks like a great addition! Apart from some nits, LGTM.
Nov 6 2017
Otherwise, from a nit that is probably an artifact from a rebase it looks good to me. But please wait for one more reviewer to accept this just to be sure.