[PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In D59692#1646990 , @martong wrote:

> Jenkins is not green 
> http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/743/
>  However, the newly failing test is `TestTargetCommand.py`, which seems to be 
> unrelated.


That ought to be fixed by r370057.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Jenkins is not green 
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/743/
However, the newly failing test is `TestTargetCommand.py`, which seems to be 
unrelated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf035b75d8f07: [ASTImporter] Fix name conflict handling with 
different strategies (authored by martong).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/test/Analysis/Inputs/ctu-other.c
  clang/unittests/AST/ASTImporterFixtures.cpp
  clang/unittests/AST/ASTImporterFixtures.h
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/include/lldb/Symbol/ClangASTImporter.h

Index: lldb/include/lldb/Symbol/ClangASTImporter.h
===
--- lldb/include/lldb/Symbol/ClangASTImporter.h
+++ lldb/include/lldb/Symbol/ClangASTImporter.h
@@ -252,7 +252,9 @@
 : clang::ASTImporter(*target_ctx, master.m_file_manager, *source_ctx,
  master.m_file_manager, true /*minimal*/),
   m_decls_to_deport(nullptr), m_decls_already_deported(nullptr),
-  m_master(master), m_source_ctx(source_ctx) {}
+  m_master(master), m_source_ctx(source_ctx) {
+  setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+}
 
 /// Scope guard that attaches a CxxModuleHandler to an ASTImporterDelegate
 /// and deattaches it at the end of the scope. Supports being used multiple
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1879,7 +1879,7 @@
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
 DeclCounter().match(ToTU, recordDecl(hasName("X";
 }
 
@@ -2432,6 +2432,62 @@
   EXPECT_EQ(ToD1, ToD2);
 }
 
+TEST_P(ImportFunctionTemplates,
+   ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void foo(T) {}
+  void foo();
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+  R"(
+  struct Foo {
+template 
+Foo(T) {}
+Foo();
+  };
+  )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+  LastDeclMatcher().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void operator<(T,T) {}
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  auto *FromD = LastDeclMatcher().match(
+  FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5127,15 +5183,6 @@
   }
 }
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
-DefaultTestValuesForRunOptions, );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
-::testing::Values(ArgVector()), );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
-::testing::Values(ArgVector()), );
-
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   Decl *FromTU = getTuDecl(
   R"(
@@ -5233,8 +5280,8 @@
   // prototype (inside 'friend') for it comes first in the AST and is not
   // linked to the definition.
   EXPECT_EQ(ImportedDef, ToClassDef);
-}  
-  
+}
+
 struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {
   LLDBLookupTest() {
 Creator = [](ASTContext , FileManager ,
@@ -5362,10 +5409,197 @@
   EXPECT_EQ(ImportedX->isAggregate(), FromX->isAggregate());
 }
 
+struct ConflictingDeclsWithLiberalStrategy : ASTImporterOptionSpecificTestBase {
+  ConflictingDeclsWithLiberalStrategy() {
+this->ODRHandling = ASTImporter::ODRHandlingType::Liberal;
+  }
+};
+
+// Check that a Decl has been successfully imported into a standalone redecl
+// chain.
+template 
+static void CheckImportedAsNew(llvm::Expected , Decl *ToTU,
+   PatternTy Pattern) {
+  ASSERT_TRUE(isSuccess(Result));
+  Decl *ImportedD = *Result;
+  

[PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 217337.
martong added a comment.

- Apply clang-format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/test/Analysis/Inputs/ctu-other.c
  clang/unittests/AST/ASTImporterFixtures.cpp
  clang/unittests/AST/ASTImporterFixtures.h
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/include/lldb/Symbol/ClangASTImporter.h

Index: lldb/include/lldb/Symbol/ClangASTImporter.h
===
--- lldb/include/lldb/Symbol/ClangASTImporter.h
+++ lldb/include/lldb/Symbol/ClangASTImporter.h
@@ -252,7 +252,9 @@
 : clang::ASTImporter(*target_ctx, master.m_file_manager, *source_ctx,
  master.m_file_manager, true /*minimal*/),
   m_decls_to_deport(nullptr), m_decls_already_deported(nullptr),
-  m_master(master), m_source_ctx(source_ctx) {}
+  m_master(master), m_source_ctx(source_ctx) {
+  setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+}
 
 /// Scope guard that attaches a CxxModuleHandler to an ASTImporterDelegate
 /// and deattaches it at the end of the scope. Supports being used multiple
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1879,7 +1879,7 @@
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
 DeclCounter().match(ToTU, recordDecl(hasName("X";
 }
 
@@ -2432,6 +2432,62 @@
   EXPECT_EQ(ToD1, ToD2);
 }
 
+TEST_P(ImportFunctionTemplates,
+   ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void foo(T) {}
+  void foo();
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+  R"(
+  struct Foo {
+template 
+Foo(T) {}
+Foo();
+  };
+  )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+  LastDeclMatcher().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void operator<(T,T) {}
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  auto *FromD = LastDeclMatcher().match(
+  FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5127,15 +5183,6 @@
   }
 }
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
-DefaultTestValuesForRunOptions, );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
-::testing::Values(ArgVector()), );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
-::testing::Values(ArgVector()), );
-
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   Decl *FromTU = getTuDecl(
   R"(
@@ -5233,8 +5280,8 @@
   // prototype (inside 'friend') for it comes first in the AST and is not
   // linked to the definition.
   EXPECT_EQ(ImportedDef, ToClassDef);
-}  
-  
+}
+
 struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {
   LLDBLookupTest() {
 Creator = [](ASTContext , FileManager ,
@@ -5362,10 +5409,197 @@
   EXPECT_EQ(ImportedX->isAggregate(), FromX->isAggregate());
 }
 
+struct ConflictingDeclsWithLiberalStrategy : ASTImporterOptionSpecificTestBase {
+  ConflictingDeclsWithLiberalStrategy() {
+this->ODRHandling = ASTImporter::ODRHandlingType::Liberal;
+  }
+};
+
+// Check that a Decl has been successfully imported into a standalone redecl
+// chain.
+template 
+static void CheckImportedAsNew(llvm::Expected , Decl *ToTU,
+   PatternTy Pattern) {
+  ASSERT_TRUE(isSuccess(Result));
+  Decl *ImportedD = *Result;
+  ASSERT_TRUE(ImportedD);
+  auto *ToD = FirstDeclMatcher().match(ToTU, Pattern);
+  EXPECT_NE(ImportedD, ToD);
+  

[PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 217330.
martong marked 2 inline comments as done.
martong added a comment.

- Revert changes in VisitFieldDecl and in VisitIndirectFieldDecl
- Remove test suite ConflictingDeclsWithConservativeStrategy


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/test/Analysis/Inputs/ctu-other.c
  clang/unittests/AST/ASTImporterFixtures.cpp
  clang/unittests/AST/ASTImporterFixtures.h
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/include/lldb/Symbol/ClangASTImporter.h

Index: lldb/include/lldb/Symbol/ClangASTImporter.h
===
--- lldb/include/lldb/Symbol/ClangASTImporter.h
+++ lldb/include/lldb/Symbol/ClangASTImporter.h
@@ -252,7 +252,9 @@
 : clang::ASTImporter(*target_ctx, master.m_file_manager, *source_ctx,
  master.m_file_manager, true /*minimal*/),
   m_decls_to_deport(nullptr), m_decls_already_deported(nullptr),
-  m_master(master), m_source_ctx(source_ctx) {}
+  m_master(master), m_source_ctx(source_ctx) {
+setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+  }
 
 /// Scope guard that attaches a CxxModuleHandler to an ASTImporterDelegate
 /// and deattaches it at the end of the scope. Supports being used multiple
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1879,7 +1879,7 @@
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
 DeclCounter().match(ToTU, recordDecl(hasName("X";
 }
 
@@ -2432,6 +2432,62 @@
   EXPECT_EQ(ToD1, ToD2);
 }
 
+TEST_P(ImportFunctionTemplates,
+   ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void foo(T) {}
+  void foo();
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+  R"(
+  struct Foo {
+template 
+Foo(T) {}
+Foo();
+  };
+  )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+  LastDeclMatcher().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void operator<(T,T) {}
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  auto *FromD = LastDeclMatcher().match(
+  FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5127,15 +5183,6 @@
   }
 }
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
-DefaultTestValuesForRunOptions, );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
-::testing::Values(ArgVector()), );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
-::testing::Values(ArgVector()), );
-
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   Decl *FromTU = getTuDecl(
   R"(
@@ -5233,8 +5280,8 @@
   // prototype (inside 'friend') for it comes first in the AST and is not
   // linked to the definition.
   EXPECT_EQ(ImportedDef, ToClassDef);
-}  
-  
+}
+
 struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {
   LLDBLookupTest() {
 Creator = [](ASTContext , FileManager ,
@@ -5362,10 +5409,205 @@
   EXPECT_EQ(ImportedX->isAggregate(), FromX->isAggregate());
 }
 
+struct ConflictingDeclsWithLiberalStrategy : ASTImporterOptionSpecificTestBase {
+  ConflictingDeclsWithLiberalStrategy() {
+this->ODRHandling = ASTImporter::ODRHandlingType::Liberal;
+  }
+};
+
+// Check that a Decl has been successfully imported into a standalone redecl
+// chain.
+template 
+static void CheckImportedAsNew(llvm::Expected , Decl *ToTU,
+   PatternTy Pattern) {
+  

[PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Gabor Marton via Phabricator via cfe-commits]

martong marked 4 inline comments as done.
martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:3452
 << FoundField->getType();
-
-  return make_error(ImportError::NameConflict);
+  ConflictingDecls.push_back(FoundField);
 }

shafik wrote:
> I am a little concerned about this change. I am wondering if this was 
> previously handled differently as a band-aid for some other issues that only 
> showed up in rare cases when they iterated over all the results but not when 
> they errored out on the first. 
> 
> Could we make the changes to `VisitFieldDecl` a separate PR so it is easier 
> to revert later on if we do find this causes a regression we are not 
> currently covering?
Ok, I reverted these hunks. I am going to create a follow-up patch which will 
include these changes.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5615
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, Typedef) {
+  Decl *ToTU = getToTuDecl(

shafik wrote:
> Since the "Liberal" strategy should be the current status quo, I think it 
> might make sense to have a first PR that just has the `*LiberalStrategy` 
> tests to verify that indeed this is the current behavior as we expect.
Ok, I removed the test suite `ConflictingDeclsWithConservativeStrategy`. I am 
going to create a subsequent patch which adds comprehensive testing for both 
strategies.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

Other than my two comment this LGTM




Comment at: clang/lib/AST/ASTImporter.cpp:3452
 << FoundField->getType();
-
-  return make_error(ImportError::NameConflict);
+  ConflictingDecls.push_back(FoundField);
 }

I am a little concerned about this change. I am wondering if this was 
previously handled differently as a band-aid for some other issues that only 
showed up in rare cases when they iterated over all the results but not when 
they errored out on the first. 

Could we make the changes to `VisitFieldDecl` a separate PR so it is easier to 
revert later on if we do find this causes a regression we are not currently 
covering?



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5615
+
+TEST_P(ConflictingDeclsWithLiberalStrategy, Typedef) {
+  Decl *ToTU = getToTuDecl(

Since the "Liberal" strategy should be the current status quo, I think it might 
make sense to have a first PR that just has the `*LiberalStrategy` tests to 
verify that indeed this is the current behavior as we expect.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

@shafik I have updated the patch with ODR handling strategies as per our 
discusson.

For the record I copy our discussion here:

On Sat, Aug 24, 2019 at 12:50 AM Shafik Yaghmour  
wrote:

> Apologies, I missed this earlier!
> 
> On Wed, Aug 21, 2019 at 2:44 AM Gábor Márton  wrote:
>  >
>  > Hi Shafik,
>  >
>  > > Right now you will end up w/ an arbitrary one of them but we do want to 
> support
>  > a way to choose between them eventually.
>  >
>  > Actually, right now (if the patch is not merged) we end up having both of 
> them in the AST. Some parts of the AST reference the existing definition, 
> while some other parts reference the new definition. Also, the regular lookup 
> will find both definitions.
>  >
>  > If the patch is merged then only the first (the existing) definition is 
> kept and an error is reported.
>  >
>  > > AFAICT this would prevent such a solution. At least that is how the
>  > new test case for RecordDecl make it appear
>  >
>  > Yes, you will never be able to remove an existing node from the AST, so I 
> don't think an either-or choosing mechanism is feasible. But you may be able 
> to decide whether you want to add the new and conflicting node. And you may 
> be able to use a different name for the new conflicting node. This may help 
> clients to see which node they are using.
>  > I want to create an additional patch which builds on this patch. Here is 
> the essence of what I'd like to have:
>  >   if (!ConflictingDecls.empty()) {
>  > Expected Resolution = Importer.HandleNameConflict(
>  > Name, DC, Decl::IDNS_Member, ConflictingDecls.data(),
>  > ConflictingDecls.size());
>  > if (Resolution)
>  >   Name = Resolution.get();
>  > else
>  >   return Resolution.takeError();
>  >   }
>  > Consider the "else" branch. I'd like to have such an "else" branch 
> everywhere. The point is to use the result of HandleNameConflict (if it is 
> set to something). This way it is possible to create any kind of ODR handling 
> by overwriting HandleNameConflict in the descendant classes.
>  >
>  > We identified 3 possible strategies so far:
>  >
>  > Conservative. In case of name conflict propagate the error. This should be 
> the default behavior.
>  > Liberal. In case of name conflict create a new node with the same name and 
> do not propagate the error.
>  > Rename. In case of name conflict create a new node with a different name 
> (e.g. with a prefix of the TU's name). Do not propagate the error.
>  >
>  >
>  > If we add a new conflicting node beside the exiting one, then some clients 
> of the AST which rely on lookup will be confused. The CTU client does not 
> rely on lookup so that is not really a problem there, but I don't know what 
> this could cause with LLDB. Perhaps the renaming strategy could work there 
> too.
>  > The Conservative and Liberal strategies are very easy to implement, and I 
> am going to create patches for that if we have consensus.
> 
> We know currently we do have cases where we have ODR violations w/
>  RecordDecl due to use of an opaque struct in the API headers and a
>  concrete instance internally e.g.:
> 
> //opaque
>  struct A {
> 
>   char buf[16];
> 
> };
> 
> //concrete
>  struct A {
> 
>   double d;
>   int64_t x;
> 
> };
> 
> and we don't want this to become an error.
> 
> I think we would at least one the choice of Conservative or Liberal to
>  be configurable and maybe LLDB would default to Liberal. This would
>  enable us to keep the status quo and not break existing cases any
>  worse than they already are.
> 
> I would prefer that would be done in this PR since I don't want to be
>  in a situation where we branch internally and we have this change but
>  not the follow-up change.
> 
>> >  I don't see how like you comment says this does not effect CXXRecordDecl
>  >
>  > In my comment I do not say that CXXRecordDecls are exceptions from the 
> general way of handling ODR violations.
>  > The comment is about that we shall not report ODR errors if we are not 
> 100% sure that we face one.
>  > So we should report an ODR error only if the found Decl and the newly 
> imported Decl have the same kind.
>  > I.e. both are CXXRecordDecls.
>  > For example, let's assume we import a CXXRecordDecl and we find an 
> existing ClassTemplateDecl with the very same Name.
>  > Then we should not report an ODR violation.
> 
> Thank you for the clarification, I misunderstood the comment, now it
>  makes more sense.
> 
>> Thanks,
>  > Gabor
>  >
>  >
>  > On Mon, Aug 19, 2019 at 5:46 PM Shafik Yaghmour 
>  wrote:
>  >>
>  >> Have a nice vacation :-)
>  >>
>  >> On Mon, Aug 19, 2019 at 8:40 AM Gábor Márton  
> wrote:
>  >> >
>  >> > Hi Shafik,
>  >> >
>  >> > I'll have an answer for you on Wednesday, I'm on vacation until then.
>  >> >
>  >> > Thanks,
>  >> > Gábor
>  >> >
>  >> > On Sat, 17 Aug 2019, 04:30 Shafik Yaghmour,  
> wrote:
>  >> >>
>  >> >> Hello Gábor,
>  >> >>
>  >> >> I was looking 

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 217135.
martong added a comment.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

- Use resulting Name from HandleNameConflict if set
- Add ODRHandling strategies
- Refactor tests, to avoid some code repetition


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/test/Analysis/Inputs/ctu-other.c
  clang/unittests/AST/ASTImporterFixtures.cpp
  clang/unittests/AST/ASTImporterFixtures.h
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/include/lldb/Symbol/ClangASTImporter.h

Index: lldb/include/lldb/Symbol/ClangASTImporter.h
===
--- lldb/include/lldb/Symbol/ClangASTImporter.h
+++ lldb/include/lldb/Symbol/ClangASTImporter.h
@@ -252,7 +252,9 @@
 : clang::ASTImporter(*target_ctx, master.m_file_manager, *source_ctx,
  master.m_file_manager, true /*minimal*/),
   m_decls_to_deport(nullptr), m_decls_already_deported(nullptr),
-  m_master(master), m_source_ctx(source_ctx) {}
+  m_master(master), m_source_ctx(source_ctx) {
+setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+  }
 
 /// Scope guard that attaches a CxxModuleHandler to an ASTImporterDelegate
 /// and deattaches it at the end of the scope. Supports being used multiple
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1879,7 +1879,7 @@
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
 DeclCounter().match(ToTU, recordDecl(hasName("X";
 }
 
@@ -2432,6 +2432,62 @@
   EXPECT_EQ(ToD1, ToD2);
 }
 
+TEST_P(ImportFunctionTemplates,
+   ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void foo(T) {}
+  void foo();
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+  R"(
+  struct Foo {
+template 
+Foo(T) {}
+Foo();
+  };
+  )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+  LastDeclMatcher().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void operator<(T,T) {}
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  auto *FromD = LastDeclMatcher().match(
+  FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5127,15 +5183,6 @@
   }
 }
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
-DefaultTestValuesForRunOptions, );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
-::testing::Values(ArgVector()), );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
-::testing::Values(ArgVector()), );
-
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   Decl *FromTU = getTuDecl(
   R"(
@@ -5233,8 +5280,8 @@
   // prototype (inside 'friend') for it comes first in the AST and is not
   // linked to the definition.
   EXPECT_EQ(ImportedDef, ToClassDef);
-}  
-  
+}
+
 struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {
   LLDBLookupTest() {
 Creator = [](ASTContext , FileManager ,
@@ -5362,10 +5409,388 @@
   EXPECT_EQ(ImportedX->isAggregate(), FromX->isAggregate());
 }
 
+// FIXME Put ODR handling strategy related tests into their own test file. And
+// create type parameterized tests for them like we do in
+// ASTImporterGenericRedeclTest.cpp
+struct ConflictingDeclsWithConservativeStrategy
+: ASTImporterOptionSpecificTestBase {};
+
+struct ConflictingDeclsWithLiberalStrategy : ASTImporterOptionSpecificTestBase {
+  ConflictingDeclsWithLiberalStrategy() {
+

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added a comment.

@shafik Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

martong marked 2 inline comments as done.
martong added inline comments.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:2392
 
+struct ImportFunctionTemplates : ASTImporterOptionSpecificTestBase {};
+

shafik wrote:
> What about tests for name conflicts for:
> 
> `NamespaceDecl` 
> `TypedefNameDecl`
> `TypeAliasTemplateDecl`
> `EnumConstantDecl`
> `RecordDecl`
> `VarDecl`
> 
> Who were also modified above.
I added several new tests with a new test suite `ConflictingDeclsTest`, they 
cover all the modifications in ASTImporter.cpp except for VarTemplateDecls.
In case of VarTemplateDecls we don't have a proper structural eq check 
implemented yet, so I disabled that test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 215579.
martong added a comment.

- Name -> SearchName
- Add tests for conflicting declarations


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/test/Analysis/Inputs/ctu-other.c
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1879,7 +1879,7 @@
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
 DeclCounter().match(ToTU, recordDecl(hasName("X";
 }
 
@@ -2432,6 +2432,62 @@
   EXPECT_EQ(ToD1, ToD2);
 }
 
+TEST_P(ImportFunctionTemplates,
+   ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void foo(T) {}
+  void foo();
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+  R"(
+  struct Foo {
+template 
+Foo(T) {}
+Foo();
+  };
+  )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+  LastDeclMatcher().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void operator<(T,T) {}
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  auto *FromD = LastDeclMatcher().match(
+  FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5127,15 +5183,6 @@
   }
 }
 
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
-DefaultTestValuesForRunOptions, );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
-::testing::Values(ArgVector()), );
-
-INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
-::testing::Values(ArgVector()), );
-
 TEST_P(ASTImporterOptionSpecificTestBase, LambdaInFunctionBody) {
   Decl *FromTU = getTuDecl(
   R"(
@@ -5233,8 +5280,8 @@
   // prototype (inside 'friend') for it comes first in the AST and is not
   // linked to the definition.
   EXPECT_EQ(ImportedDef, ToClassDef);
-}  
-  
+}
+
 struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {
   LLDBLookupTest() {
 Creator = [](ASTContext , FileManager ,
@@ -5362,10 +5409,214 @@
   EXPECT_EQ(ImportedX->isAggregate(), FromX->isAggregate());
 }
 
+struct ConflictingDeclsTest : ASTImporterOptionSpecificTestBase {};
+
+TEST_P(ConflictingDeclsTest, Typedef) {
+  getToTuDecl(
+  R"(
+  typedef int X;
+  )",
+  Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+  R"(
+  typedef double X;
+  )",
+  Lang_CXX11);
+  auto *FromX = FirstDeclMatcher().match(
+  FromTU, typedefNameDecl(hasName("X")));
+  auto *ImportedX = Import(FromX, Lang_CXX11);
+  EXPECT_FALSE(ImportedX);
+  ASTImporter *Importer = findFromTU(FromX)->Importer.get();
+  Optional OptErr = Importer->getImportDeclErrorIfAny(FromX);
+  ASSERT_TRUE(OptErr);
+  EXPECT_EQ(OptErr->Error, ImportError::NameConflict);
+}
+
+TEST_P(ConflictingDeclsTest, TypeAlias) {
+  getToTuDecl(
+  R"(
+  using X = int;
+  )",
+  Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+  R"(
+  using X = double;
+  )",
+  Lang_CXX11);
+  auto *FromX = FirstDeclMatcher().match(
+  FromTU, typedefNameDecl(hasName("X")));
+  auto *ImportedX = Import(FromX, Lang_CXX11);
+  EXPECT_FALSE(ImportedX);
+  ASTImporter *Importer = findFromTU(FromX)->Importer.get();
+  Optional OptErr = Importer->getImportDeclErrorIfAny(FromX);
+  ASSERT_TRUE(OptErr);
+  EXPECT_EQ(OptErr->Error, ImportError::NameConflict);
+}
+
+TEST_P(ConflictingDeclsTest, EnumDecl) {
+  getToTuDecl(
+  R"(
+  enum X { a, b };
+  )",
+  Lang_CXX11);
+  Decl *FromTU = getTuDecl(
+  R"(
+  enum X { a, b, c };
+  )",
+  Lang_CXX11);
+  auto *FromX = 

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added a comment.

In D59692#1632138 , @shafik wrote:

> Just wanted to see if you were planning on landing this soon, the fix `Name` 
> -> `SearchName` is probably an important one since we have seen several 
> issues w/ bugs like that but I really would like to see more tests. Are you 
> having issues coming up w/ tests?


Hey Shafik, I was busy lately with some non-ASTImporter related tasks, but just 
recently I could drive myself back to it.
I am planning to add the test cases in the following work days, but the latest 
by the end of the next week.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Just wanted to see if you were planning on landing this soon, the fix `Name` -> 
`SearchName` is probably an important one since we have seen several issues w/ 
bugs like that but I really would like to see more tests. Are you having issues 
coming up w/ tests?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

First round of review.




Comment at: clang/lib/AST/ASTImporter.cpp:2632
+  ExpectedName NameOrErr = Importer.HandleNameConflict(
+  Name, DC, IDNS, ConflictingDecls.data(), ConflictingDecls.size());
+  if (!NameOrErr)

`Name` -> `SearchName`



Comment at: clang/unittests/AST/ASTImporterTest.cpp:2392
 
+struct ImportFunctionTemplates : ASTImporterOptionSpecificTestBase {};
+

What about tests for name conflicts for:

`NamespaceDecl` 
`TypedefNameDecl`
`TypeAliasTemplateDecl`
`EnumConstantDecl`
`RecordDecl`
`VarDecl`

Who were also modified above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 211312.
martong added a comment.

- Rebase to master


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/test/Analysis/Inputs/ctu-other.c
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1879,7 +1879,7 @@
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
 DeclCounter().match(ToTU, recordDecl(hasName("X";
 }
 
@@ -2389,6 +2389,64 @@
 functionDecl(hasName("f"), hasDescendant(declRefExpr()));
 }
 
+struct ImportFunctionTemplates : ASTImporterOptionSpecificTestBase {};
+
+TEST_P(ImportFunctionTemplates,
+   ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void foo(T) {}
+  void foo();
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+  R"(
+  struct Foo {
+template 
+Foo(T) {}
+Foo();
+  };
+  )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+  LastDeclMatcher().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void operator<(T,T) {}
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  auto *FromD = LastDeclMatcher().match(
+  FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5207,6 +5265,9 @@
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctions,
 DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionTemplates,
+DefaultTestValuesForRunOptions, );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctionTemplates,
 DefaultTestValuesForRunOptions, );
 
Index: clang/test/Analysis/Inputs/ctu-other.c
===
--- clang/test/Analysis/Inputs/ctu-other.c
+++ clang/test/Analysis/Inputs/ctu-other.c
@@ -12,11 +12,11 @@
 }
 
 // Test enums.
-enum B { x = 42,
- l,
- s };
+enum B { x2 = 42,
+ y2,
+ z2 };
 int enumCheck(void) {
-  return x;
+  return x2;
 }
 
 // Test reporting an error in macro definition
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -80,6 +80,7 @@
   using ExpectedExpr = llvm::Expected;
   using ExpectedDecl = llvm::Expected;
   using ExpectedSLoc = llvm::Expected;
+  using ExpectedName = llvm::Expected;
 
   std::string ImportError::toString() const {
 // FIXME: Improve error texts.
@@ -2188,11 +2189,11 @@
 }
 
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Namespace,
- ConflictingDecls.data(),
- ConflictingDecls.size());
-  if (!Name)
-return make_error(ImportError::NameConflict);
+  ExpectedName NameOrErr = Importer.HandleNameConflict(
+  Name, DC, Decl::IDNS_Namespace, ConflictingDecls.data(),
+  ConflictingDecls.size());
+  if (!NameOrErr)
+return NameOrErr.takeError();
 }
   }
 
@@ -2296,21 +2297,19 @@
   // already have a complete underlying type then return with that.
   if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType())
 return Importer.MapImported(D, FoundTypedef);
+  // FIXME Handle redecl chain. When you do that make consistent changes
+  // in ASTImporterLookupTable too.
+} else {
+  ConflictingDecls.push_back(FoundDecl);
 }
-// FIXME 

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Ping


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Ping @a_sidorin @shafik


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 194527.
martong marked an inline comment as done.
martong added a comment.

- Put back the call to GetOriginalDecl


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp
  test/Analysis/Inputs/ctu-other.c
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1982,7 +1982,7 @@
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
 DeclCounter().match(ToTU, recordDecl(hasName("X";
 }
 
@@ -2692,6 +2692,64 @@
 functionDecl(hasName("f"), hasDescendant(declRefExpr()));
 }
 
+struct ImportFunctionTemplates : ASTImporterOptionSpecificTestBase {};
+
+TEST_P(ImportFunctionTemplates,
+   ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void foo(T) {}
+  void foo();
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+  R"(
+  struct Foo {
+template 
+Foo(T) {}
+Foo();
+  };
+  )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+  LastDeclMatcher().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void operator<(T,T) {}
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  auto *FromD = LastDeclMatcher().match(
+  FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5552,6 +5610,9 @@
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctions,
 DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionTemplates,
+DefaultTestValuesForRunOptions, );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctionTemplates,
 DefaultTestValuesForRunOptions, );
 
Index: test/Analysis/Inputs/ctu-other.c
===
--- test/Analysis/Inputs/ctu-other.c
+++ test/Analysis/Inputs/ctu-other.c
@@ -12,11 +12,11 @@
 }
 
 // Test enums.
-enum B { x = 42,
- l,
- s };
+enum B { x2 = 42,
+ y2,
+ z2 };
 int enumCheck(void) {
-  return x;
+  return x2;
 }
 
 // Test reporting an error in macro definition
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -79,6 +79,7 @@
   using ExpectedExpr = llvm::Expected;
   using ExpectedDecl = llvm::Expected;
   using ExpectedSLoc = llvm::Expected;
+  using ExpectedName = llvm::Expected;
 
   std::string ImportError::toString() const {
 // FIXME: Improve error texts.
@@ -2135,11 +2136,11 @@
 }
 
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Namespace,
- ConflictingDecls.data(),
- ConflictingDecls.size());
-  if (!Name)
-return make_error(ImportError::NameConflict);
+  ExpectedName NameOrErr = Importer.HandleNameConflict(
+  Name, DC, Decl::IDNS_Namespace, ConflictingDecls.data(),
+  ConflictingDecls.size());
+  if (!NameOrErr)
+return NameOrErr.takeError();
 }
   }
 
@@ -2243,20 +2244,17 @@
   // already have a complete underlying type then return with that.
   if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType())
 return Importer.MapImported(D, FoundTypedef);
+} else {
+  ConflictingDecls.push_back(FoundDecl);
 }
-// FIXME Handle redecl chain.
-break;
   }
-
-  ConflictingDecls.push_back(FoundDecl);
 }
 
 if (!ConflictingDecls.empty()) {
-  Name = 

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

martong marked 2 inline comments as done.
martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:2154
+return NameOrErr.takeError();
 }
   }

a_sidorin wrote:
> Should we write `else Name = *NameOrError`?
Atm, we implement the simplest strategy for name conflict handling: we just 
return with the error.
However, on a long term we should use the returned name indeed. I mean when we 
really do implement a renaming strategy via `HandleNameConflict`. 

Also, for that we'd have to double check that the `Name` is indeed used when we 
create the AST node. So I'd rather leave this `else` branch up to that point. 
Hopefully, by that time we'll have unittests which would exercise this `else` 
branch, now we just don't have any.



Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Thanks for the fixes! I'd like to clarify one moment, however.




Comment at: lib/AST/ASTImporter.cpp:2154
+return NameOrErr.takeError();
 }
   }

Should we write `else Name = *NameOrError`?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 192263.
martong marked 2 inline comments as done.
martong added a comment.

- Add braces around else
- Remove falsely duplicated push_back
- Use Expected in HandleNameConflict


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp
  test/Analysis/Inputs/ctu-other.c
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1964,7 +1964,7 @@
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
 DeclCounter().match(ToTU, recordDecl(hasName("X";
 }
 
@@ -2674,6 +2674,64 @@
 functionDecl(hasName("f"), hasDescendant(declRefExpr()));
 }
 
+struct ImportFunctionTemplates : ASTImporterOptionSpecificTestBase {};
+
+TEST_P(ImportFunctionTemplates,
+   ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void foo(T) {}
+  void foo();
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+  R"(
+  struct Foo {
+template 
+Foo(T) {}
+Foo();
+  };
+  )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+  LastDeclMatcher().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void operator<(T,T) {}
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  auto *FromD = LastDeclMatcher().match(
+  FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5534,6 +5592,9 @@
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctions,
 DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionTemplates,
+DefaultTestValuesForRunOptions, );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctionTemplates,
 DefaultTestValuesForRunOptions, );
 
Index: test/Analysis/Inputs/ctu-other.c
===
--- test/Analysis/Inputs/ctu-other.c
+++ test/Analysis/Inputs/ctu-other.c
@@ -12,11 +12,11 @@
 }
 
 // Test enums.
-enum B { x = 42,
- l,
- s };
+enum B { x2 = 42,
+ y2,
+ z2 };
 int enumCheck(void) {
-  return x;
+  return x2;
 }
 
 // Test reporting an error in macro definition
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -79,6 +79,7 @@
   using ExpectedExpr = llvm::Expected;
   using ExpectedDecl = llvm::Expected;
   using ExpectedSLoc = llvm::Expected;
+  using ExpectedName = llvm::Expected;
 
   std::string ImportError::toString() const {
 // FIXME: Improve error texts.
@@ -1944,15 +1945,6 @@
 
 bool ASTNodeImporter::IsStructuralMatch(RecordDecl *FromRecord,
 RecordDecl *ToRecord, bool Complain) {
-  // Eliminate a potential failure point where we attempt to re-import
-  // something we're trying to import while completing ToRecord.
-  Decl *ToOrigin = Importer.GetOriginalDecl(ToRecord);
-  if (ToOrigin) {
-auto *ToOriginRecord = dyn_cast(ToOrigin);
-if (ToOriginRecord)
-  ToRecord = ToOriginRecord;
-  }
-
   StructuralEquivalenceContext Ctx(Importer.getFromContext(),
ToRecord->getASTContext(),
Importer.getNonEquivalentDecls(),
@@ -2154,11 +2146,11 @@
 }
 
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Namespace,
- ConflictingDecls.data(),
- ConflictingDecls.size());
-  if (!Name)
-return 

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

martong marked 5 inline comments as done.
martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:2260
 
   ConflictingDecls.push_back(FoundDecl);
 }

a_sidorin wrote:
> Do we push the same decl twice?
Uhh, thanks, that is rebase error I've made.



Comment at: lib/AST/ASTImporter.cpp:8532
 unsigned NumDecls) {
-  return Name;
+  return DeclarationName();
 }

a_sidorin wrote:
> Empty DeclarationName can be valid sometimes. Should we return 
> ErrorOr instead? This can also simplify caller code a bit.
That's a good idea, thanks.
Though, I'd use Expected rather, that would be more consistent with the rest 
of the code. 


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,

Thank you for addressing the problem!




Comment at: lib/AST/ASTImporter.cpp:2256
 return Importer.MapImported(D, FoundTypedef);
-}
-// FIXME Handle redecl chain.
-break;
+} else
+  ConflictingDecls.push_back(FoundDecl);

`if` body is surrounded by braces, so it's better to surround `else` too.



Comment at: lib/AST/ASTImporter.cpp:2260
 
   ConflictingDecls.push_back(FoundDecl);
 }

Do we push the same decl twice?



Comment at: lib/AST/ASTImporter.cpp:8532
 unsigned NumDecls) {
-  return Name;
+  return DeclarationName();
 }

Empty DeclarationName can be valid sometimes. Should we return 
ErrorOr instead? This can also simplify caller code a bit.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: a_sidorin, shafik, teemperor.
Herald added subscribers: cfe-commits, jdoerfert, gamesh411, Szelethus, dkrupp, 
rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a project: clang.
martong added a parent revision: D55049: Changed every use of 
ASTImporter::Import to Import_New.

There are numorous flaws about the name conflict handling, this patch
attempts fixes them. Changes in details:

- HandleNameConflict return with a false DeclarationName

Hitherto we effectively never returned with a NameConflict error, even
if the preceding StructuralMatch indicated a conflict.
Because we just simply returned with the parameter `Name` in
HandleNameConflict and that name is almost always `true` when converted to
`bool`.

- Add tests which indicate wrong NameConflict handling

- Add to ConflictingDecls only if decl kind is different

Note, we might not indicate an ODR error when there is an existing record decl
and a enum is imported with same name.  But there are other cases. E.g. think
about the case when we import a FunctionTemplateDecl with name f and we found a
simple FunctionDecl with name f. They overload.  Or in case of a
ClassTemplateDecl and CXXRecordDecl, the CXXRecordDecl could be the 'templated'
class, so it would be false to report error.  So I think we should report a
name conflict error only when we are 100% sure of that.  That is why I think it
should be a general pattern to report the error only if the kind is the same.

- Fix failing ctu test with EnumConstandDecl

In ctu-main.c we have the enum class 'A' which brings in the enum
constant 'x' with value 0 into the global namespace.
In ctu-other.c we had the enum class 'B' which brought in the same name
('x') as an enum constant but with a different enum value (42). This is clearly
an ODR violation in the global namespace. The solution was to rename the
second enum constant.

- Fix lldb test failures

Remove the call of the unused and vexing GetOriginalDecl(). This information is
already maintained in the ImportedDecls member.


Repository:
  rC Clang

https://reviews.llvm.org/D59692

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp
  test/Analysis/Inputs/ctu-other.c
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1964,7 +1964,7 @@
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
 DeclCounter().match(ToTU, recordDecl(hasName("X";
 }
 
@@ -2674,6 +2674,64 @@
 functionDecl(hasName("f"), hasDescendant(declRefExpr()));
 }
 
+struct ImportFunctionTemplates : ASTImporterOptionSpecificTestBase {};
+
+TEST_P(ImportFunctionTemplates,
+   ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void foo(T) {}
+  void foo();
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+  R"(
+  struct Foo {
+template 
+Foo(T) {}
+Foo();
+  };
+  )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+  LastDeclMatcher().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void operator<(T,T) {}
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  auto *FromD = LastDeclMatcher().match(
+  FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5534,6 +5592,9 @@
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctions,
 DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionTemplates,
+DefaultTestValuesForRunOptions, );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctionTemplates,
 DefaultTestValuesForRunOptions, );
 
Index: test/Analysis/Inputs/ctu-other.c