[clang] [clang][ASTImporter] Fix import of variable template redeclarations. (PR #72841)

2024-01-15 Thread Balázs Kéri via cfe-commits 

https://github.com/balazske closed 
https://github.com/llvm/llvm-project/pull/72841
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][ASTImporter] Fix import of variable template redeclarations. (PR #72841)

2024-01-15 Thread via cfe-commits 
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 


https://github.com/NagyDonat approved this pull request.

Thanks for the clarifications, LGTM. As others had lots of opportunities to 
comment on this change, I think it's safe to merge this now (assuming that the 
tests are OK).

https://github.com/llvm/llvm-project/pull/72841
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][ASTImporter] Fix import of variable template redeclarations. (PR #72841)

2024-01-12 Thread Balázs Kéri via cfe-commits 


@@ -6245,17 +6245,21 @@ ExpectedDecl 
ASTNodeImporter::VisitVarTemplateDecl(VarTemplateDecl *D) {
   D->getTemplatedDecl()))
 continue;
   if (IsStructuralMatch(D, FoundTemplate)) {
-// The Decl in the "From" context has a definition, but in the
-// "To" context we already have a definition.
+// FIXME Check for ODR error if the two definitions have
+// different initializers?
 VarTemplateDecl *FoundDef = getTemplateDefinition(FoundTemplate);
-if (D->isThisDeclarationADefinition() && FoundDef)
-  // FIXME Check for ODR error if the two definitions have
-  // different initializers?
-  return Importer.MapImported(D, FoundDef);
-if (FoundTemplate->getDeclContext()->isRecord() &&
-D->getDeclContext()->isRecord())
-  return Importer.MapImported(D, FoundTemplate);
-
+if (D->getDeclContext()->isRecord()) {
+  assert(FoundTemplate->getDeclContext()->isRecord() &&
+ "Member variable template imported as non-member, "
+ "inconsistent imported AST?");

balazske wrote:

The existing "To" AST can be somehow invalid or declarations can be in wrong 
scope, because previous wrong AST imports and structural equivalence problems, 
or here a wrong declaration may be found. This assertion looks to check for 
such problems (check `FoundTemplate->getDeclContext()->isRecord()` if 
`D->getDeclContext()->isRecord()`).

https://github.com/llvm/llvm-project/pull/72841
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][ASTImporter] Fix import of variable template redeclarations. (PR #72841)

2024-01-09 Thread via cfe-commits 
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -6245,17 +6245,21 @@ ExpectedDecl 
ASTNodeImporter::VisitVarTemplateDecl(VarTemplateDecl *D) {
   D->getTemplatedDecl()))
 continue;
   if (IsStructuralMatch(D, FoundTemplate)) {
-// The Decl in the "From" context has a definition, but in the
-// "To" context we already have a definition.
+// FIXME Check for ODR error if the two definitions have
+// different initializers?
 VarTemplateDecl *FoundDef = getTemplateDefinition(FoundTemplate);
-if (D->isThisDeclarationADefinition() && FoundDef)
-  // FIXME Check for ODR error if the two definitions have
-  // different initializers?
-  return Importer.MapImported(D, FoundDef);
-if (FoundTemplate->getDeclContext()->isRecord() &&
-D->getDeclContext()->isRecord())
-  return Importer.MapImported(D, FoundTemplate);
-
+if (D->getDeclContext()->isRecord()) {
+  assert(FoundTemplate->getDeclContext()->isRecord() &&
+ "Member variable template imported as non-member, "
+ "inconsistent imported AST?");

NagyDonat wrote:

Why is this assert always valid? I thought about this for some time and I 
couldn't rule out that this would crash on some tricky code.


https://github.com/llvm/llvm-project/pull/72841
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][ASTImporter] Fix import of variable template redeclarations. (PR #72841)

2023-12-19 Thread Balázs Kéri via cfe-commits 


@@ -5050,6 +5050,59 @@ TEST_P(ImportFriendClasses, RecordVarTemplateDecl) {
   EXPECT_EQ(ToTUX, ToX);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateDeclConflict) {
+  getToTuDecl(
+  R"(
+  template 
+  constexpr int X = 1;
+  )",
+  Lang_CXX14);
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  template 
+  constexpr int X = 2;
+  )",
+  Lang_CXX14, "input1.cc");
+  auto *FromX = FirstDeclMatcher().match(
+  FromTU, varTemplateDecl(hasName("X")));
+  auto *ToX = Import(FromX, Lang_CXX11);
+  // FIXME: This import should fail.
+  EXPECT_TRUE(ToX);

balazske wrote:

This import should not fail (it is no error to compile code with a variable 
template that is declared as extern and has a non-extern definition, and the 
"extern" part could be in an include file), and the same does not fail with 
non-template variables. This case should be checked and the structural 
equivalence of variable templates is missing now, but that change could go into 
a new pull request.

https://github.com/llvm/llvm-project/pull/72841
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][ASTImporter] Fix import of variable template redeclarations. (PR #72841)

2023-12-13 Thread Qizhi Hu via cfe-commits 
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -5050,6 +5050,59 @@ TEST_P(ImportFriendClasses, RecordVarTemplateDecl) {
   EXPECT_EQ(ToTUX, ToX);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateDeclConflict) {
+  getToTuDecl(
+  R"(
+  template 
+  constexpr int X = 1;
+  )",
+  Lang_CXX14);
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  template 
+  constexpr int X = 2;
+  )",
+  Lang_CXX14, "input1.cc");
+  auto *FromX = FirstDeclMatcher().match(
+  FromTU, varTemplateDecl(hasName("X")));
+  auto *ToX = Import(FromX, Lang_CXX11);
+  // FIXME: This import should fail.
+  EXPECT_TRUE(ToX);

jcsxky wrote:

If import `X` should fail, What about this case in 
`ASTImporterGenericRedeclTest.cpp`
```cpp
struct VariableTemplate {
  using DeclTy = VarTemplateDecl;
  static constexpr auto *Prototype = "template  extern T X;";
  static constexpr auto *Definition =
  R"(
  template  T X;
  template <> int X;
  )";
  // There is no matcher for varTemplateDecl so use a work-around.
  BindableMatcher getPattern() {
return namedDecl(hasName("X"), unless(isImplicit()),
 has(templateTypeParmDecl()));
  }
};
```
Storage of `X` in `Prototype` and `Definition` is different, it should fail 
when imported.
I added structural equivalence of `VarTemplateDecl` and makes this import 
failed in `VarTemplateDeclConflict`. is it also need to fix the testcase in 
`ASTImporterGenericRedeclTest.cpp`?
```cpp
static bool IsStructurallyEquivalent(StructuralEquivalenceContext ,
 VarTemplateDecl *D1,
 VarTemplateDecl *D2) {
  if (!IsStructurallyEquivalent(Context, D1->getDeclName(), D2->getDeclName()))
return false;

  // Check the templated declaration.
  if (!IsStructurallyEquivalent(Context, D1->getTemplatedDecl(),
D2->getTemplatedDecl()))
return false;

  // Check template parameters.
  return IsStructurallyEquivalent(Context, D1->getTemplateParameters(),
  D2->getTemplateParameters());
}
```

https://github.com/llvm/llvm-project/pull/72841
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][ASTImporter] Fix import of variable template redeclarations. (PR #72841)

2023-12-05 Thread Balázs Kéri via cfe-commits 


@@ -5050,6 +5050,59 @@ TEST_P(ImportFriendClasses, RecordVarTemplateDecl) {
   EXPECT_EQ(ToTUX, ToX);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateDeclConflict) {
+  getToTuDecl(
+  R"(
+  template 
+  constexpr int X = 1;
+  )",
+  Lang_CXX14);
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  template 
+  constexpr int X = 2;
+  )",
+  Lang_CXX14, "input1.cc");
+  auto *FromX = FirstDeclMatcher().match(
+  FromTU, varTemplateDecl(hasName("X")));
+  auto *ToX = Import(FromX, Lang_CXX11);
+  // FIXME: This import should fail.
+  EXPECT_TRUE(ToX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateStaticDefinition) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  struct A {
+template 
+static int X;
+  };
+  )",
+  Lang_CXX14);
+  auto *ToX = FirstDeclMatcher().match(
+  ToTU, varTemplateDecl(hasName("X")));
+  ASSERT_FALSE(ToX->isThisDeclarationADefinition());
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct A {
+template 

balazske wrote:

Yes, this code was incorrect. It is still a bug that the test did not fail, my 
next planned fixes should improve this situation.

https://github.com/llvm/llvm-project/pull/72841
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][ASTImporter] Fix import of variable template redeclarations. (PR #72841)

2023-12-05 Thread Balázs Kéri via cfe-commits 

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/72841

From 99d6169f62862b7b1147da7fd26a85df20a0aba5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Mon, 20 Nov 2023 10:14:52 +0100
Subject: [PATCH 1/3] [clang][ASTImporter] Fix import of variable template
 redeclarations.

In some cases variable templates (specially if static member of record)
were not correctly imported and an assertion "Missing call to MapImported?"
could happen.
---
 clang/lib/AST/ASTImporter.cpp   |  27 +++---
 clang/unittests/AST/ASTImporterTest.cpp | 105 
 2 files changed, 122 insertions(+), 10 deletions(-)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index c4e931e220f69..7a5e3d6653285 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -6245,17 +6245,21 @@ ExpectedDecl 
ASTNodeImporter::VisitVarTemplateDecl(VarTemplateDecl *D) {
   D->getTemplatedDecl()))
 continue;
   if (IsStructuralMatch(D, FoundTemplate)) {
-// The Decl in the "From" context has a definition, but in the
-// "To" context we already have a definition.
+// FIXME Check for ODR error if the two definitions have
+// different initializers?
 VarTemplateDecl *FoundDef = getTemplateDefinition(FoundTemplate);
-if (D->isThisDeclarationADefinition() && FoundDef)
-  // FIXME Check for ODR error if the two definitions have
-  // different initializers?
-  return Importer.MapImported(D, FoundDef);
-if (FoundTemplate->getDeclContext()->isRecord() &&
-D->getDeclContext()->isRecord())
-  return Importer.MapImported(D, FoundTemplate);
-
+if (D->getDeclContext()->isRecord()) {
+  assert(FoundTemplate->getDeclContext()->isRecord() &&
+ "Member variable template imported as non-member, "
+ "inconsistent imported AST?");
+  if (FoundDef)
+return Importer.MapImported(D, FoundDef);
+  if (!D->isThisDeclarationADefinition())
+return Importer.MapImported(D, FoundTemplate);
+} else {
+  if (FoundDef && D->isThisDeclarationADefinition())
+return Importer.MapImported(D, FoundDef);
+}
 FoundByLookup = FoundTemplate;
 break;
   }
@@ -6374,7 +6378,10 @@ ExpectedDecl 
ASTNodeImporter::VisitVarTemplateSpecializationDecl(
 // variable.
 return Importer.MapImported(D, FoundDef);
   }
+  // FIXME HandleNameConflict
+  return make_error(ASTImportError::NameConflict);
 }
+return Importer.MapImported(D, D2);
   } else {
 TemplateArgumentListInfo ToTAInfo;
 if (const ASTTemplateArgumentListInfo *Args = D->getTemplateArgsInfo()) {
diff --git a/clang/unittests/AST/ASTImporterTest.cpp 
b/clang/unittests/AST/ASTImporterTest.cpp
index 5f4d8d040772c..d439a14b7b998 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -5050,6 +5050,111 @@ TEST_P(ImportFriendClasses, RecordVarTemplateDecl) {
   EXPECT_EQ(ToTUX, ToX);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateDeclConflict) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  template 
+  constexpr int X = 1;
+  )",
+  Lang_CXX14);
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  template 
+  constexpr int X = 2;
+  )",
+  Lang_CXX14, "input1.cc");
+  auto *FromX = FirstDeclMatcher().match(
+  FromTU, varTemplateDecl(hasName("X")));
+  auto *ToX = Import(FromX, Lang_CXX11);
+  // FIXME: This import should fail.
+  EXPECT_TRUE(ToX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateStaticDefinition) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  struct A {
+template 
+static int X;
+  };
+  )",
+  Lang_CXX14);
+  auto *ToX = FirstDeclMatcher().match(
+  ToTU, varTemplateDecl(hasName("X")));
+  ASSERT_FALSE(ToX->isThisDeclarationADefinition());
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct A {
+template 
+static int X;
+  };
+  template 
+  int A::X = 2;
+  )",
+  Lang_CXX14, "input1.cc");
+  auto *FromXDef = LastDeclMatcher().match(
+  FromTU, varTemplateDecl(hasName("X")));
+  ASSERT_TRUE(FromXDef->isThisDeclarationADefinition());
+  auto *ToXDef = Import(FromXDef, Lang_CXX14);
+  EXPECT_TRUE(ToXDef);
+  EXPECT_TRUE(ToXDef->isThisDeclarationADefinition());
+  EXPECT_EQ(ToXDef->getPreviousDecl(), ToX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateSpecializationDeclValue) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  template 
+  constexpr int X = U::Value;
+  struct A { static constexpr int Value = 1; };
+  constexpr int Y = X;
+  )",
+  Lang_CXX14);
+
+  auto *ToTUX = FirstDeclMatcher().match(
+  ToTU, varTemplateSpecializationDecl(hasName("X")));
+  Decl

[clang] [clang][ASTImporter] Fix import of variable template redeclarations. (PR #72841)

2023-12-01 Thread Shafik Yaghmour via cfe-commits 
=?utf-8?q?Bal=C3=A1zs_K=C3=A9ri?= 
Message-ID:
In-Reply-To: 



@@ -5050,6 +5050,59 @@ TEST_P(ImportFriendClasses, RecordVarTemplateDecl) {
   EXPECT_EQ(ToTUX, ToX);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateDeclConflict) {
+  getToTuDecl(
+  R"(
+  template 
+  constexpr int X = 1;
+  )",
+  Lang_CXX14);
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  template 
+  constexpr int X = 2;
+  )",
+  Lang_CXX14, "input1.cc");
+  auto *FromX = FirstDeclMatcher().match(
+  FromTU, varTemplateDecl(hasName("X")));
+  auto *ToX = Import(FromX, Lang_CXX11);
+  // FIXME: This import should fail.
+  EXPECT_TRUE(ToX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateStaticDefinition) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  struct A {
+template 
+static int X;
+  };
+  )",
+  Lang_CXX14);
+  auto *ToX = FirstDeclMatcher().match(
+  ToTU, varTemplateDecl(hasName("X")));
+  ASSERT_FALSE(ToX->isThisDeclarationADefinition());
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct A {
+template 

shafik wrote:

Did you mean to drop the `class V` here? 

https://github.com/llvm/llvm-project/pull/72841
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][ASTImporter] Fix import of variable template redeclarations. (PR #72841)

2023-11-23 Thread Balázs Kéri via cfe-commits 

balazske wrote:

I plan to fix import of `VarTemplateSpecializationDecl` in a different PR. The 
indicated assertion "Missing call to MapImported?" is likely to related to this 
part.

https://github.com/llvm/llvm-project/pull/72841
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][ASTImporter] Fix import of variable template redeclarations. (PR #72841)

2023-11-22 Thread Balázs Kéri via cfe-commits 

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/72841

From 99d6169f62862b7b1147da7fd26a85df20a0aba5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Mon, 20 Nov 2023 10:14:52 +0100
Subject: [PATCH 1/2] [clang][ASTImporter] Fix import of variable template
 redeclarations.

In some cases variable templates (specially if static member of record)
were not correctly imported and an assertion "Missing call to MapImported?"
could happen.
---
 clang/lib/AST/ASTImporter.cpp   |  27 +++---
 clang/unittests/AST/ASTImporterTest.cpp | 105 
 2 files changed, 122 insertions(+), 10 deletions(-)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index c4e931e220f69b5..7a5e3d665328532 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -6245,17 +6245,21 @@ ExpectedDecl 
ASTNodeImporter::VisitVarTemplateDecl(VarTemplateDecl *D) {
   D->getTemplatedDecl()))
 continue;
   if (IsStructuralMatch(D, FoundTemplate)) {
-// The Decl in the "From" context has a definition, but in the
-// "To" context we already have a definition.
+// FIXME Check for ODR error if the two definitions have
+// different initializers?
 VarTemplateDecl *FoundDef = getTemplateDefinition(FoundTemplate);
-if (D->isThisDeclarationADefinition() && FoundDef)
-  // FIXME Check for ODR error if the two definitions have
-  // different initializers?
-  return Importer.MapImported(D, FoundDef);
-if (FoundTemplate->getDeclContext()->isRecord() &&
-D->getDeclContext()->isRecord())
-  return Importer.MapImported(D, FoundTemplate);
-
+if (D->getDeclContext()->isRecord()) {
+  assert(FoundTemplate->getDeclContext()->isRecord() &&
+ "Member variable template imported as non-member, "
+ "inconsistent imported AST?");
+  if (FoundDef)
+return Importer.MapImported(D, FoundDef);
+  if (!D->isThisDeclarationADefinition())
+return Importer.MapImported(D, FoundTemplate);
+} else {
+  if (FoundDef && D->isThisDeclarationADefinition())
+return Importer.MapImported(D, FoundDef);
+}
 FoundByLookup = FoundTemplate;
 break;
   }
@@ -6374,7 +6378,10 @@ ExpectedDecl 
ASTNodeImporter::VisitVarTemplateSpecializationDecl(
 // variable.
 return Importer.MapImported(D, FoundDef);
   }
+  // FIXME HandleNameConflict
+  return make_error(ASTImportError::NameConflict);
 }
+return Importer.MapImported(D, D2);
   } else {
 TemplateArgumentListInfo ToTAInfo;
 if (const ASTTemplateArgumentListInfo *Args = D->getTemplateArgsInfo()) {
diff --git a/clang/unittests/AST/ASTImporterTest.cpp 
b/clang/unittests/AST/ASTImporterTest.cpp
index 5f4d8d040772cb1..d439a14b7b9985f 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -5050,6 +5050,111 @@ TEST_P(ImportFriendClasses, RecordVarTemplateDecl) {
   EXPECT_EQ(ToTUX, ToX);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateDeclConflict) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  template 
+  constexpr int X = 1;
+  )",
+  Lang_CXX14);
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  template 
+  constexpr int X = 2;
+  )",
+  Lang_CXX14, "input1.cc");
+  auto *FromX = FirstDeclMatcher().match(
+  FromTU, varTemplateDecl(hasName("X")));
+  auto *ToX = Import(FromX, Lang_CXX11);
+  // FIXME: This import should fail.
+  EXPECT_TRUE(ToX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateStaticDefinition) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  struct A {
+template 
+static int X;
+  };
+  )",
+  Lang_CXX14);
+  auto *ToX = FirstDeclMatcher().match(
+  ToTU, varTemplateDecl(hasName("X")));
+  ASSERT_FALSE(ToX->isThisDeclarationADefinition());
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct A {
+template 
+static int X;
+  };
+  template 
+  int A::X = 2;
+  )",
+  Lang_CXX14, "input1.cc");
+  auto *FromXDef = LastDeclMatcher().match(
+  FromTU, varTemplateDecl(hasName("X")));
+  ASSERT_TRUE(FromXDef->isThisDeclarationADefinition());
+  auto *ToXDef = Import(FromXDef, Lang_CXX14);
+  EXPECT_TRUE(ToXDef);
+  EXPECT_TRUE(ToXDef->isThisDeclarationADefinition());
+  EXPECT_EQ(ToXDef->getPreviousDecl(), ToX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateSpecializationDeclValue) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  template 
+  constexpr int X = U::Value;
+  struct A { static constexpr int Value = 1; };
+  constexpr int Y = X;
+  )",
+  Lang_CXX14);
+
+  auto *ToTUX = FirstDeclMatcher().match(
+  ToTU, varTemplateSpecializationDecl(hasName("X")));
+

[clang] [clang][ASTImporter] Fix import of variable template redeclarations. (PR #72841)

2023-11-20 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Balázs Kéri (balazske)


Changes

In some cases variable templates (specially if static member of record) were 
not correctly imported and an assertion "Missing call to MapImported?" could 
happen.

---
Full diff: https://github.com/llvm/llvm-project/pull/72841.diff


2 Files Affected:

- (modified) clang/lib/AST/ASTImporter.cpp (+17-10) 
- (modified) clang/unittests/AST/ASTImporterTest.cpp (+105) 


``diff
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index c4e931e220f69b5..7a5e3d665328532 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -6245,17 +6245,21 @@ ExpectedDecl 
ASTNodeImporter::VisitVarTemplateDecl(VarTemplateDecl *D) {
   D->getTemplatedDecl()))
 continue;
   if (IsStructuralMatch(D, FoundTemplate)) {
-// The Decl in the "From" context has a definition, but in the
-// "To" context we already have a definition.
+// FIXME Check for ODR error if the two definitions have
+// different initializers?
 VarTemplateDecl *FoundDef = getTemplateDefinition(FoundTemplate);
-if (D->isThisDeclarationADefinition() && FoundDef)
-  // FIXME Check for ODR error if the two definitions have
-  // different initializers?
-  return Importer.MapImported(D, FoundDef);
-if (FoundTemplate->getDeclContext()->isRecord() &&
-D->getDeclContext()->isRecord())
-  return Importer.MapImported(D, FoundTemplate);
-
+if (D->getDeclContext()->isRecord()) {
+  assert(FoundTemplate->getDeclContext()->isRecord() &&
+ "Member variable template imported as non-member, "
+ "inconsistent imported AST?");
+  if (FoundDef)
+return Importer.MapImported(D, FoundDef);
+  if (!D->isThisDeclarationADefinition())
+return Importer.MapImported(D, FoundTemplate);
+} else {
+  if (FoundDef && D->isThisDeclarationADefinition())
+return Importer.MapImported(D, FoundDef);
+}
 FoundByLookup = FoundTemplate;
 break;
   }
@@ -6374,7 +6378,10 @@ ExpectedDecl 
ASTNodeImporter::VisitVarTemplateSpecializationDecl(
 // variable.
 return Importer.MapImported(D, FoundDef);
   }
+  // FIXME HandleNameConflict
+  return make_error(ASTImportError::NameConflict);
 }
+return Importer.MapImported(D, D2);
   } else {
 TemplateArgumentListInfo ToTAInfo;
 if (const ASTTemplateArgumentListInfo *Args = D->getTemplateArgsInfo()) {
diff --git a/clang/unittests/AST/ASTImporterTest.cpp 
b/clang/unittests/AST/ASTImporterTest.cpp
index 5f4d8d040772cb1..d439a14b7b9985f 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -5050,6 +5050,111 @@ TEST_P(ImportFriendClasses, RecordVarTemplateDecl) {
   EXPECT_EQ(ToTUX, ToX);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateDeclConflict) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  template 
+  constexpr int X = 1;
+  )",
+  Lang_CXX14);
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  template 
+  constexpr int X = 2;
+  )",
+  Lang_CXX14, "input1.cc");
+  auto *FromX = FirstDeclMatcher().match(
+  FromTU, varTemplateDecl(hasName("X")));
+  auto *ToX = Import(FromX, Lang_CXX11);
+  // FIXME: This import should fail.
+  EXPECT_TRUE(ToX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateStaticDefinition) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  struct A {
+template 
+static int X;
+  };
+  )",
+  Lang_CXX14);
+  auto *ToX = FirstDeclMatcher().match(
+  ToTU, varTemplateDecl(hasName("X")));
+  ASSERT_FALSE(ToX->isThisDeclarationADefinition());
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct A {
+template 
+static int X;
+  };
+  template 
+  int A::X = 2;
+  )",
+  Lang_CXX14, "input1.cc");
+  auto *FromXDef = LastDeclMatcher().match(
+  FromTU, varTemplateDecl(hasName("X")));
+  ASSERT_TRUE(FromXDef->isThisDeclarationADefinition());
+  auto *ToXDef = Import(FromXDef, Lang_CXX14);
+  EXPECT_TRUE(ToXDef);
+  EXPECT_TRUE(ToXDef->isThisDeclarationADefinition());
+  EXPECT_EQ(ToXDef->getPreviousDecl(), ToX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateSpecializationDeclValue) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  template 
+  constexpr int X = U::Value;
+  struct A { static constexpr int Value = 1; };
+  constexpr int Y = X;
+  )",
+  Lang_CXX14);
+
+  auto *ToTUX = FirstDeclMatcher().match(
+  ToTU, varTemplateSpecializationDecl(hasName("X")));
+  Decl *FromTU = getTuDecl(
+  R"(
+  template 
+  constexpr int X = U::Value;
+  struct A { static constexpr int Value = 1; };
+  constexpr int Y = X;
+  )",
+  Lang_CXX14,

[clang] [clang][ASTImporter] Fix import of variable template redeclarations. (PR #72841)

2023-11-20 Thread Balázs Kéri via cfe-commits 

https://github.com/balazske created 
https://github.com/llvm/llvm-project/pull/72841

In some cases variable templates (specially if static member of record) were 
not correctly imported and an assertion "Missing call to MapImported?" could 
happen.

From 99d6169f62862b7b1147da7fd26a85df20a0aba5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Mon, 20 Nov 2023 10:14:52 +0100
Subject: [PATCH] [clang][ASTImporter] Fix import of variable template
 redeclarations.

In some cases variable templates (specially if static member of record)
were not correctly imported and an assertion "Missing call to MapImported?"
could happen.
---
 clang/lib/AST/ASTImporter.cpp   |  27 +++---
 clang/unittests/AST/ASTImporterTest.cpp | 105 
 2 files changed, 122 insertions(+), 10 deletions(-)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index c4e931e220f69b5..7a5e3d665328532 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -6245,17 +6245,21 @@ ExpectedDecl 
ASTNodeImporter::VisitVarTemplateDecl(VarTemplateDecl *D) {
   D->getTemplatedDecl()))
 continue;
   if (IsStructuralMatch(D, FoundTemplate)) {
-// The Decl in the "From" context has a definition, but in the
-// "To" context we already have a definition.
+// FIXME Check for ODR error if the two definitions have
+// different initializers?
 VarTemplateDecl *FoundDef = getTemplateDefinition(FoundTemplate);
-if (D->isThisDeclarationADefinition() && FoundDef)
-  // FIXME Check for ODR error if the two definitions have
-  // different initializers?
-  return Importer.MapImported(D, FoundDef);
-if (FoundTemplate->getDeclContext()->isRecord() &&
-D->getDeclContext()->isRecord())
-  return Importer.MapImported(D, FoundTemplate);
-
+if (D->getDeclContext()->isRecord()) {
+  assert(FoundTemplate->getDeclContext()->isRecord() &&
+ "Member variable template imported as non-member, "
+ "inconsistent imported AST?");
+  if (FoundDef)
+return Importer.MapImported(D, FoundDef);
+  if (!D->isThisDeclarationADefinition())
+return Importer.MapImported(D, FoundTemplate);
+} else {
+  if (FoundDef && D->isThisDeclarationADefinition())
+return Importer.MapImported(D, FoundDef);
+}
 FoundByLookup = FoundTemplate;
 break;
   }
@@ -6374,7 +6378,10 @@ ExpectedDecl 
ASTNodeImporter::VisitVarTemplateSpecializationDecl(
 // variable.
 return Importer.MapImported(D, FoundDef);
   }
+  // FIXME HandleNameConflict
+  return make_error(ASTImportError::NameConflict);
 }
+return Importer.MapImported(D, D2);
   } else {
 TemplateArgumentListInfo ToTAInfo;
 if (const ASTTemplateArgumentListInfo *Args = D->getTemplateArgsInfo()) {
diff --git a/clang/unittests/AST/ASTImporterTest.cpp 
b/clang/unittests/AST/ASTImporterTest.cpp
index 5f4d8d040772cb1..d439a14b7b9985f 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -5050,6 +5050,111 @@ TEST_P(ImportFriendClasses, RecordVarTemplateDecl) {
   EXPECT_EQ(ToTUX, ToX);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateDeclConflict) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  template 
+  constexpr int X = 1;
+  )",
+  Lang_CXX14);
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  template 
+  constexpr int X = 2;
+  )",
+  Lang_CXX14, "input1.cc");
+  auto *FromX = FirstDeclMatcher().match(
+  FromTU, varTemplateDecl(hasName("X")));
+  auto *ToX = Import(FromX, Lang_CXX11);
+  // FIXME: This import should fail.
+  EXPECT_TRUE(ToX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateStaticDefinition) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  struct A {
+template 
+static int X;
+  };
+  )",
+  Lang_CXX14);
+  auto *ToX = FirstDeclMatcher().match(
+  ToTU, varTemplateDecl(hasName("X")));
+  ASSERT_FALSE(ToX->isThisDeclarationADefinition());
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct A {
+template 
+static int X;
+  };
+  template 
+  int A::X = 2;
+  )",
+  Lang_CXX14, "input1.cc");
+  auto *FromXDef = LastDeclMatcher().match(
+  FromTU, varTemplateDecl(hasName("X")));
+  ASSERT_TRUE(FromXDef->isThisDeclarationADefinition());
+  auto *ToXDef = Import(FromXDef, Lang_CXX14);
+  EXPECT_TRUE(ToXDef);
+  EXPECT_TRUE(ToXDef->isThisDeclarationADefinition());
+  EXPECT_EQ(ToXDef->getPreviousDecl(), ToX);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateSpecializationDeclValue) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  template 
+  constexpr int X = U::Value;
+  struct A { static constexpr int Value = 1; };
+