- User Since
- Sep 17 2012, 3:16 AM (309 w, 1 d)
Wed, Aug 15
Generally looks good, I only wonder if this works well with inline namespaces. Could you test?
Thu, Aug 2
We could also print something about the ReturnStmt, like source location or pretty print of its expressions so we can check that we picked the right one in case we have multiple.
But consider this as an optional task if you have nothing better to do. I am ok with committing this as is.
Fri, Jul 27
Wed, Jul 25
Small comments inline.
Mon, Jul 23
Some comments, mostly nits inline.
Jul 19 2018
Yeah, I would rather have the cleanups and do extra work in the visitor. But lets wait what @NoQ thinks.
Jul 11 2018
Jul 9 2018
Don't you need to edit the tests as well?
Jul 8 2018
Thanks! The changes look good, I forgot to mark one double lookup though in my previous review.
Overall looks good, some nits inline. Let's run it on some projects to exercise this change.
Jul 4 2018
Jun 26 2018
Ok, it looks like naively just dropping all the constraints at Z3 is not the most efficient way.
Jun 25 2018
Jun 24 2018
LGTM, given that the assert does not fire for the projects you tested on.
Jun 23 2018
Looks better, thanks!
Regarding the visitor:
Maybe rather than looking at the AST, we should check the states, when we started to track the returned symbol?
Jun 18 2018
I wonder if this could be done when plist is emitted generally, independent of any checks.
Jun 13 2018
Jun 11 2018
Having C++ support would be awesome!
Thanks for working on this!
Jun 10 2018
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
Jun 5 2018
Jun 4 2018
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).
Jun 1 2018
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?
May 30 2018
May 26 2018
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.