- User Since
- Sep 17 2012, 3:16 AM (317 w, 6 d)
Thu, Oct 18
Wed, Oct 10
I am not sure what to do about implcit checks. Those are probably should never be turned on or off by the user, but they should be on or off by default based on the set of checks the user enabled and the platform she is using. Thus, I am perfectly ok with the implicit_checks.html only being accessible from the checker development manual. Maybe we should extend the checker list with a notice that the user should never disable the core checks.
Tue, Oct 9
Mon, Oct 8
- Added the ideas from Kristof.
I agree that it would make sense to either not have arguments that are not represented in the AST or create expressions for those implicit arguments.
Sep 6 2018
I like that we print the program points for hidden nodes :) Great work!
Aug 21 2018
Oh, and thanks for working on this, this improvement was long overdue, but everybody was busy with something else :)
Sorry for the delay, I think this is OK to commit.
As a possible improvement, I can imagine making it slightly stricter, e.g. you could only skip QualifiedNames for inline namespaces and the beginning. Maybe add support for staring with "" or :: to even disable skipping namespaces at the beginning?
But these are just nice to have features, I am perfectly ok with not having them or doing it in a followup patch.
Aug 15 2018
Generally looks good, I only wonder if this works well with inline namespaces. Could you test? Probably it will.
Aug 2 2018
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.
Jul 27 2018
Jul 25 2018
Small comments inline.
Jul 23 2018
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.