- User Since
- Sep 17 2012, 3:16 AM (287 w, 3 d)
Overall looks good, some minor comments inline.
Mon, Mar 19
I was thinking about test/Analysis/lambdas.cpp as a possible candidate.
Sat, Mar 17
I am only wondering what is the policy regarding the tests? When should we add a new file or when should we just extend an existing one?
Tue, Mar 6
Hmm, it looks like these changes are already upstreamed. Probably as part of other patch or maybe just someone (probably me) forgot to put the proper revision in the commit message? I close this revision now.
I think having both kinds of tests might make sense.
Overall, this looks good to me. Some nits inline.
Note that changes from https://reviews.llvm.org/D44100 might affect this.
Fri, Mar 2
Resubmitted in https://reviews.llvm.org/rL326439
Thu, Mar 1
Could you please provide some more details where the cyclic dependency is? I cannot reproduce the problem and usually cmake fails when there is a cyclic dependency and you are building shared libraries.
Wed, Feb 28
Feb 19 2018
Could you add some context?
Feb 15 2018
- Rebased to current ToT
- Fixed a problem that the scan-build-py used an old version of the ctu configuration option
- Added a short guide how to use CTU
Feb 12 2018
@alexfh did you have any chance to think about this change?
Feb 9 2018
I like the idea of having more statistics. Moreover, due to the fact that we output the percent of reachable basic blocks as an integer, it was really hard to get precise coverage data. This addition will help in this regard.
Why not turn this on by default when we turn on reporting statistics?
Having a test for the plist output would be nice.
Thanks, this looks good to me! I will try this out soon and commit after that.
Looks good to me. Only found a few nits.
Feb 7 2018
Feb 6 2018
Overall looks good. Was this tested on large software? I would also be grateful if you could run the regression tests with templight always being enabled to see if they uncover any assertions/crashes.
Feb 4 2018
Jan 25 2018
- Address review comments.
Overall looks good! Thanks for working on this!
Jan 24 2018
Jan 23 2018
- Added test for CXXTypeIdExpr import.
- Added import for CXXTypeidExpr. What is the best way to test this? <typeinfo> header is required for using the typeid operator, but relying on the presence of an STL library in tests is usually considered as a bad practice. Should we mock a typeinfo implementation just for this purpose?
I do not see a test for the following changes:
- ASTImporter: don't add templated declarations into DeclContext
- ASTImporter: proper set ParmVarDecls for imported FunctionProtoTypeLoc
High level note: clang::TypeAliasDecl has the same issue as CXXRecordDecl, so templated versions should not be added to the DeclContext. Could you add that just for the sake of completeness?
Jan 22 2018
- Address review comments.
Jan 20 2018
This should be rebased to latest master.
Jan 11 2018
- Fixed review comments
Jan 9 2018
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.
Jan 8 2018
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).
Jan 6 2018
https://reviews.llvm.org/D41538 is superior and committed.
Jan 4 2018
- Address some review comments.
Dec 27 2017
- Address review comments.
Dec 22 2017
In case you do not like this solution, I uploaded an alternative approach: https://reviews.llvm.org/D41538
Dec 21 2017
Dec 20 2017
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.
Dec 19 2017
- 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?