Is it a better fix to change getMacroInfoForLocation?

static const MacroInfo *getMacroInfoForLocation(const Preprocessor &PP,
                                                const SourceManager &SM,
                                                const IdentifierInfo *II,
                                                SourceLocation Loc) {

The following ASTImporterTest reproduces the assert:

TEST_P(ASTImporterOptionSpecificTestBase, SourceLocationDifferentTU) {
  TranslationUnitDecl *ToTU = getToTuDecl("void f1();", Lang_CXX11);
  Decl *FromTU = getTuDecl("void f2();", Lang_CXX11, "input.cc");
  auto *ToF1 = FirstDeclMatcher<FunctionDecl>().match(ToTU, functionDecl());
  auto *FromF2 = FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl());

The problem has not necessarily do something with macro expansion. If a function is imported in CTU mode and for any reason source locations inside it are compared against source locations in the original TU these locations are (from SourceManager point of view) in different TUs. But from TU point of view these are in the same TU because the AST import (AST import keeps the original filename and location in the new source location, so the new SourceLocation is in the same TU but different source file). Maybe this is a new case related to CTU mode in the compare function so it is correct to handle this case too (without assert).

Added lit tests for the simple (Decl) cases. The Expr and type related changes are more tricky to do. But I do not like this approach to add new tests because the test function in D60463 does almost the same thing for every import in ASTTests without adding lot of new checks manually.

• Import BraceRange of EnumDecl.
• Added lit tests.

• Using size_t instead of int.

In the current state there are failing AST tests. This test can be added after the problems are fixed.

New patch and check the first line of AST dump only.

It looks like that the same test can be applied as in D60463 but check only the first line of the AST dump. The first line contains information about the actual Decl only. This checks less than checking the full AST dump but finds some of wrong SourceLocation import. The problem is that the test can not be added yet because there are other failures. At least one new patch is needed with corrections related to import of TypeSourceInfo. Anyway this change (and the others) can not be added together with the test because multiple patches (including this) are needed to make the test not fail.

There is a test for the SourceLocation import in https://reviews.llvm.org/D60463 (after this patch is applied that test should not fail and the "return" statement is to be removed).

• Import BraceRange of EnumDecl.

• Changed type of string constants to const.

Thanks for reviewing.

Maybe this patch can be abandoned, the correction is already made (when every _New was removed).

ping

Rebase to current master.

This test will work theoretically only if the order of every imported Decl (and not only FieldDecl) is correct, this is not the case now. So probably a better solution for the problem should be found: Enumerate and match (the From and To) SourceLocations with AST visitor. There should be some existing code that is doing somewhat similar in clang-query but I did not find it.

• Removed unneeded and wrong type cast.

Corrected rebase problems.

• Rebase to current master.
• Fixed error handling in testImport.

I would prefer an observer-like solution but I am not against this change.

By the way if the Import of the strategy uses recursive import of other things this can cause same problems as it was in ASTImporter before the GetImportedOrCreateDecl was introduced. So this should be avoided or something similar to GetImportedOrCreateDecl must be performed, maybe that function can be made public somehow.

Another observation: The Import function of the strategy now has no way to return an error. An even better version of it would be to include the possibility of import error (with ImportError, or other error type). Or the "PreImport" function could indicate if the Decl is handled by the strategy, and Import is called only if yes and Import can return an Expected<Decl *>. (The PostImport is called for every successfully imported Decl.)

It looks better now.
One "problem" is that now there is the strategy and there is the Imported function. It would be possible to unify these into a strategy that contains maybe a "import" and a "post-import" callback. (But this requires big changes in the ASTImporter user code.)

The replace of Import calls may work if the internal state of the ASTImporter is updated correctly from the 'shim' object (especially if not every Decl is handled by the shim). For this to work some of the state of ASTImporter that was intended to be internal must be made public, for example map of imported decls and lookup table. The shim may corrupt the ASTImporter state for example when the lookup table is not updated correctly. (Probably for the LLDB special use cases this is not a problem?)

I do not know if "Shim" is the correct name for this, maybe "Strategy" is better?

Rebase.

This is the next part of the changes for error handling. D53818 was for ASTImporter functions only, this is for ASTNodeImporter (the Import_New functions are not used at many places directly because mostly the few import templates are used). The next part is D55049 which switches to Import_New in every other place (in clang source code only). Next part would be to use Import_New in lldb, then rename all Import_New to Import (and remove old Import).

Rebase (old patch applied without changes).

• Small style corrections.

Looks good now.

Currently there is no direct test for ASTImporter FileID (and other non-Decl) import. These are tested indirectly by other import tests. There is something that fails in a macOS LLDB test without this.

This change is needed for D53818 to make LLDB tests pass.

So the problem is that there are references to ParmVarDecl from inside function body and at import of ParmVarDecl always a new one is created even if there is an already existing (in the existing function prototype)? Maybe it works in VisitParmVarDecl to search for already existing ParmVarDecl (not already imported) and return if found.

• Small style corrections.

Rebase.