I can confirm the issue with my patch, so this piece of code needs to be removed.

This should be trivial enough to just commit, but I'd just be more comfortable if someone looked at it before, because this is my first commit in this area of clang.

@xazax.hun Yes, I planned to just commit. Set you as Subscriber not Reviewer in Arc. Was just a bit confused why it fails at first :)

Was already done: https://github.com/llvm-mirror/clang/commit/bacdda22396c39181aa0e641182e01a0b3cf43ea

@xazax.hun good point and that actually fixes a bug since that branch should also do the deep check. Added that to the tests.

• addressed review comments
• created cross_tu::containsConst
• fixed bug that unions were not exported
• added TODO test for constructor case

ASTContext::getParents should not be used this way. This use-case is solved by function-scoped ParentMaps or AnalysisDeclContext::getParentMap. Discussion: http://lists.llvm.org/pipermail/cfe-dev/2019-April/061944.html

In D46421#1456171, @xazax.hun wrote:

My last set of comments are also unresolved. Most of them are minor nits, but I would love to get rid of the code duplication between ClangExtDefMapGen and the Clang Static Analyzer regarding when do we consider a variable worth to import. Otherwise the patch looks good to me.

Okay so I would suggest to go ahead and commit this. Rebased it succeeds without modification.

Thanks for the comments! All good points. Nice idea with the constructor, but that can probably happen in a follow up patch.

In D46421#1374807, @NoQ wrote:

At the same time, i don't have any test cases for the actual change in behavior that such canonicalization causes. If the test case that you had in mind is indeed demonstrating this problem, i'd love to have it. If it turns out that your test case doesn't allow us to demonstrate the problem without CTU, then probably it has something to do with ASTImporter accidentally canonicalizing the the declaration in DeclRefExpr more rarely than the vanilla AST.

Strip name changes (see D56441); addressed review comments

addressed review comments

Thanks for the quick response! Will wait a couple days to see if someone else has something to add.

I tried adding isGLValue to evalStore and the test suite didn't complain. For evalLoad (on BoundEx) it failed in pretty much every test. Should the evalStore assert also go into trunk?

rebased

Please let me know if there is anything else that should be addressed.

cfe-dev thread with short discussion: http://lists.llvm.org/pipermail/cfe-dev/2018-April/057562.html

@rsmith Yes, this should generally better be handled outside of the ASTContext. That would be out of scope of this patch. Is it fine to proceed with this one for now?

Alright, but then I would suggest to pass an invalid loc to the constructors instead to make it more explicit and save a map lookup in the import function.

In D47698#1141871, @r.stahl wrote:

improved code quality; added nested macro test. it "works", but is disabled because it revealed another bug: the function end location is not imported. will send a patch

@rsmith do you have a chance to take a look or assign someone else?

improved code quality; added nested macro test. it "works", but is disabled because it revealed another bug: the function end location is not imported. will send a patch

In D47698#1140629, @thakis wrote:

This code is live when reading pchs, correct? Does this have any measurable perf impact on deserializing pchs for, say, Cocoa.h or Windows.h?

addressed review comment, but there is nothing like FullSourceRange to improve everything

In D30691#879838, @xazax.hun wrote:

In D30691#878830, @r.stahl wrote:

For my purposes I replaced the return statement of the compareCrossTUSourceLocs function with:

return XL.getFileID() < YL.getFileID();

A more correct fix would create only one unique diagnostic for both cases.

Thank you for the report, I could reproduce this after modifying the null dereference checker. My fear of using file IDs is that I don't know whether they are stable. So in subsequent runs, different paths might be chosen by the analyzer and this could be confusing to the user. I will think about a workaround that both stable and solves this assertion.

I see two possible ways to do the proper fix. One is to check explicitly for this case when the same function appears in multiple translation units. A better approach would be to have the ASTImporter handle this case. I think the proper fix is better addressed in a separate patch.

remove stray include

• add missing AnalysisConsumer diff
• only collect const qualified vardecls in the extdef index and adjust test

In D46421#1119098, @xazax.hun wrote:

Sorry for the limited activity. Unfortunately, I have very little time reviewing patches lately.

The split-token case should be covered by this, because it takes the correct TokenLen and the isTokenRange flag. Other than that the split-token ExpansionInfo is indistinguishable.

Added FIXME tests.

I'm not really too confident to approve changes yet, but with a second opinion it should be fine.

addressed review comments

updated test

I looked through the original patches and found quite a few more occurrences of "function map" and renamed them - including the tool "clang-func-mapping". There is one comment "by using the 'clang-extdef-mapping' packaged with Xcode (on OS X)". Is this implicit from the install targets that the tool name changed or do I have to inform someone?

• added tests
• updated doc and var naming
• addressed review comment

In D45774#1088453, @NoQ wrote:

Two questions for the future:

• Should we skip the initializer binding for local variables (and fields/elements of local variables) entirely? Cause we can load from them anyway. And keeping the Store small might be good for performance.

Didn't see that overload, thanks!

full patch with test

Tests and doc fixes pending. First I'm interested in your thoughts to this change.

addressed the comments, thanks!

Maybe this is a user error of CrossTU, but it seemed to import a FuncDecl with attributes, causing the imported FuncDecl to have all those attributes twice. That's why I thought merging would maybe make sense. However I did not encounter any issue with the duplicate attributes. Only the wrong source locations produced odd crashes.