- User Since
- Sep 17 2012, 3:16 AM (300 w, 3 d)
Mon, Jun 18
I wonder if this could be done when plist is emitted generally, independent of any checks.
Wed, Jun 13
Mon, Jun 11
Having C++ support would be awesome!
Thanks for working on this!
Sun, Jun 10
Just for the record, there is a common example where relying on copy elision might bite and google do not recommend relying on it for correctness: https://abseil.io/tips/120
Tue, Jun 5
Mon, Jun 4
So having an analyzer-config option would be useful for those codebases that are expected to be compiled with less advanced compilers that do not elide copies or not doing it aggressively enough. Maybe it is worth to ask on the mailing list of the community wants to have an option for this? I am not sure though how often the end users end up fine tuning the analyzer. It might be nice to have a guide how to do that (and in what circumstances does each of the config values make sense).
Fri, Jun 1
Sorry for the limited activity. Unfortunately, I have very little time reviewing patches lately.
I think we need to answer the following questions:
- Does this change affect the analysis of the constructors of global objects? If so, how?
- Do we want to import non-const object's initializer expressions? The analyzer might not end up using the value anyways.
- How big can the index get with this modification for large projects?
Wed, May 30
Sat, May 26
Looks good so far, some comments inline.
May 14 2018
The analyzer can only reason about const variables this way, right? Maybe we should only import the initializers for such variables to have better performance? What do you think?
May 5 2018
- Rerun script
Apr 29 2018
Apr 25 2018
Isn't this case already covered by conversion checker? https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
With a sufficiently detailed commit message, i.e.: what version of a project should be cheked out and how the analyzer needs to be ivoked to reproduce the problem I am ok with committing this without a test.
Apr 24 2018
Apr 16 2018
I am in favour of this approach. This is what I suggested back than in https://reviews.llvm.org/D23014 but it was somehow overlooked.
Apr 13 2018
Apr 12 2018
Overall looks good, some minor nits inline. If those can be resolved trivially, I am ok with committing this without another round of reviews.
We encountered the same problem but did not have time yet to submit the patch. We have literally the same fix internally, so it looks good to me. One minor style nit inline.
Mar 30 2018
Mar 29 2018
You should always upload the whole diff, not just the last changes.
Mar 22 2018
Overall looks good, some minor comments inline.
Mar 19 2018
I was thinking about test/Analysis/lambdas.cpp as a possible candidate.
Mar 17 2018
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?
Mar 6 2018
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.
Mar 2 2018
Resubmitted in https://reviews.llvm.org/rL326439
Mar 1 2018
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.
Feb 28 2018
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.