Generally looks good, I only wonder if this works well with inline namespaces. Could you test?

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.

LGTM!

Small comments inline.

In D49693#1172605, @mikhail.ramalho wrote:

What I'm suggesting is: first run all visitors, since no other visitor mark them as invalid, the evaluation of all 497 bugs will be fast and will return only on bug report. We run the refutation manager in this one bug report.

Some comments, mostly nits inline.

Yeah, I would rather have the cleanups and do extra work in the visitor. But lets wait what @NoQ thinks.

In D49074#1159095, @george.karpenkov wrote:

This may remove false-positives (like the example above), but we surely cannot find new errors where multiplicative operations are used.

Do you have examples of those? In my understanding, (almost) all of the imprecisions in the solver lead purely to false positives.
The only example where you lose bugs is when you stop exploring due to reaching a (false) fatal error.

Don't you need to edit the tests as well?

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.

In D48027#1142324, @MTC wrote:

• There is possible match the wrong AST node, e.g. for the NamedDecl foo(), if the function body has the a::b::x, when we match a::b::X(), we will match foo() . Because I'm not familiar with ASTMatcher, my bad, I can't think of a perfect way to match only the specified nodes.

Ok, it looks like naively just dropping all the constraints at Z3 is not the most efficient way.

LG!

LGTM!

LGTM, given that the assert does not fire for the projects you tested on.

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?

I wonder if this could be done when plist is emitted generally, independent of any checks.

Having C++ support would be awesome!
Thanks for working on this!

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

In D47671#1122994, @george.karpenkov wrote:

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.

I'm not sure it makes sense. From my understanding, that's just how c++17 is defined, and that's unrelated to what compiler implementation is used.

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).

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?

In D45517#1116770, @george.karpenkov wrote:

I am not not sure that I got the idea what are you suggesting here. If we have the constraint of for example a symbol s > 10 and later on a path we discover s > 20, will we also deduplicate this that way?

No. But I thought in your optimization atoms inside the constraints would be the same?
Could you give an example where they are not?

In D45517#1116734, @george.karpenkov wrote:

@xazax.hun (I'll reply here to avoid scattering the conversation across many subtrees)

I was thinking about the optimization for not adding redundant constraints some more, and I've decided I'm still against it ---
we are creating a higher potential for bugs, and we are tightly coupling the visitor to an internal implementation detail (all formulas are eventually purged at purge locations),
which creates a more fragile code.

The proper way to do this would be to have a set of constraints, and then add all constraints there as we iterate through the states (and through constraints inside the state).
If we use the hashing function provided by Z3, the simple act of construction of a set would implicitly drop all redundant constraints.

LG!

LGTM!

Looks good so far, some comments inline.

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?

• Rerun script

In D46234#1082307, @rsmith wrote:

Can you say something about why you want to track this?

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.

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.

Overall looks good, some minor nits inline. If those can be resolved trivially, I am ok with committing this without another round of reviews.

In D45458#1062342, @NoQ wrote:

@xazax.hun: do you think the approach you took in the Valist checker is applicable here? Did you like how it ended up working? Cause i'd love to see CallDescription and initializer lists used for readability here. And i'd love to see branches replaced with map lookups. And i'm not sure if anybody has written code that has all the stuff that i'd love to see.

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.

LG! Thanks!

You should always upload the whole diff, not just the last changes.

Overall looks good, some minor comments inline.

I was thinking about test/Analysis/lambdas.cpp as a possible candidate.

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?

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.

Hi Aleksei!

Resubmitted in https://reviews.llvm.org/rL326439

LGTM!

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.

Could you add some context?

In D30691#1003514, @george.karpenkov wrote:

Python code looks OK to me, I have one last request: could we have a small documentation how the whole thing is supposed work in integration, preferably on an available open-source project any reader could check out?
I am asking because I have actually tried and failed to launch this CTU patch on a project I was analyzing.

• 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

@alexfh did you have any chance to think about this change?

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.

In D43131#1003607, @NoQ wrote:

So are these present only on translation units that actually caused bug reports to appear?

Having a test for the plist output would be nice.

I suspect such test would be super fragile. Every time we change any modeling, many of the stats may change.

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.

In D5767#999143, @sabel83 wrote:

2. What do you mean by regression tests? We have run the clang-test target successfully on the patched code (which has the hook). Note that the hook this pull request provides is implemented as a ProgramAction. It is not intended to be used together with other program actions, I don't see how we could turn it on for other tests (if that is what you referred to).

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.

• Address review comments.

Overall looks good! Thanks for working on this!

In D42301#986583, @a.sidorin wrote:

I'd rather create a separate patch - this one is already large enough. Is it OK?

• 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?

In D42301#984713, @a.sidorin wrote:

I do not see a test for the following changes:

• ASTImporter: don't add templated declarations into DeclContext

It's in ASTImporterTest. It checks that the templated decl cannot be found in the enclosing TU.

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?

In D42335#983878, @a.sidorin wrote:

Hello Peter,

Thank you for the patch! It is almost LGTM, just a few minor questions inline.
Am I understand correctly that it is partially based on https://github.com/haoNoQ/clang/blob/summary-ipa-draft/lib/AST/ASTImporter.cpp?
Also, FYI: you can use ASTMerge and clang-import-test facilities for testing. ASTMatchers are completely fine, but usage of imported AST samples as Sema lookup source can uncover some subtle issues in the built AST.

• Address review comments.