• Encapsulate by adding addDeclToLookup and removeDeclFromLookup to the shared state

• Set error for FromD if it maps to an existing Decl which has an error set

• Add FIXMEs
• Set error for FromD if it maps to an existing Decl which has an error set
• Add test

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/29310/ is green.

@shafik I've been looking for any lldb regression in our Mac machine, could not find any. Now I am looking at http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ . I don't expect regression because here we changed logic about the error handling only, so I'd expect to have regression only if we had testcases in lldb for erroneous cases, but apparently there are no such tests.

• Use make_scope_exit
• Make hasCycleAtBack const

• Use make_scope_exit
• Add test ErrorIsNotPropagatedFromMemberToNamespace

• Add back an assertion in setImportDeclError(), remove the condition in Import()

• Remove the macro and use std::string.op+
• We may set up an error twice in case of a cycle, thus remove the assert

• Remove the macro and use std::string.op+

• Assert that we set the error only once
• Remove the macro and use std::string.op+

I think I still owe you some explanation about this patch.
I do consider this patch as one of the most intricate patches regarding ASTImporter.
I'd like to answer the following questions in this comment:
What is an ImportPath and why do we need to track it?
What does it mean if we have a cycle in the import path?
How do we use these cycles during the error handling?

Thanks for the review!
I have provided test cases for the 3 previous patches on which this one depends on. I will provide additional tests next week for this one, and of course will address the other comments.

Thank you for the review, I have added a test which demonstrates the changes.
By tracking the import paths and cycles we implement a very strict error handling mechanism, but this seems to be the way to avoid reaching any faulty AST nodes for the ASTImporter clients.

• Add test ErrorPropagatesThroughImportCycles
• Change existing test to new behaviour

• Remove unused include and using

• Remove formatv b/c it can't handle braces in code

• Remove formatv b/c it can't handle braces in code

• Remove unrelated change

• Test error values are set for AST nodes

About the regression of TestFormatters.py: I realized that the problem is about the wrong implementation of the ExternalASTSource interface.
In the implementation of FindExternalLexicalDecls of this interface, we simply ignored those cases when the given predicate (passed as a param) is false. When that happens, that means we still have some more external decls which should be dug up by the upcoming calls of DeclContext::lookup. The fix is about to indicate this.

• Fix regression of TestFormatters.py

• Simplify getRecordDeclOfFriend

Looks good, thank you!

The ASTImporter.cpp looks good to me. (Becasue the BranchHint is a simple an enum, so we don't need to specifically import that as we would in case of e.g. an Expr.)

• Remove superflous '.c_str()'

• Change to if(Log *log = ...)
• Use LLDB_LOG

• Remove getUnderlyingType

• se -> so

• Rebase to master
• Rebase to D62061

• Address Shafik's comments and update assertions

• Rebase to master

In rC354839#649398, @rnk wrote:

In rC354839#649303, @martong wrote:

This is a destructive revert commit which does not simply revert the mentioned r354832 commit. Besides reverting that commit, it does remove a bunch of unrelated test files! E.g. cfe/trunk/test/ASTMerge/var/Inputs/var1.c. Could you please investigate and restore the unrelated test files?

Sorry about that! I added the files back in rC360699, but some of the tests now fail, so I XFAIL'd them and applied the fixes separately in rC360701 (and forgot one change, which became rC360705). It looks like you made the behavior change in rC357394 that would've caused these tests to start failing, so you might want to take a look. It's also worth pointing out that clang/test/ASTMerge/class-template-partial-specialization/test.cpp crashes clang right now. It may have been a regression from April, but I couldn't say for sure.

I might try to reproduce the git-llvm bug that caused the file deletion if I have time, but more likely I won't get to it, and I'll just have to be more careful when reverting things in the future.

Thank you guys for the review!

Please ignore my previous comment, I didn't recognize that the fact of the revert was indeed commented at the patch, it is my bad.

This is a destructive revert commit which does not simply revert the mentioned r354832 commit. Besides reverting that commit, it does remove a bunch of unrelated test files! E.g. cfe/trunk/test/ASTMerge/var/Inputs/var1.c. Could you please investigate and restore the unrelated test files?

@aprantl Ping