[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Jenkins looks okay: 
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31160/ .The build is 
red but the the previous run has the very same failing test case:
expression_command/weak_symbols/TestWeakSymbols.py


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64075



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

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL366332: [ASTImporter] Fix structural eq of lambdas (authored 
by martong, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64075?vs=208727=210326#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64075

Files:
  cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp
  cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp

Index: cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
===
--- cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
+++ cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
@@ -1085,6 +1085,19 @@
   return true;
 }
 
+/// Determine structural equivalence of two lambda classes.
+static bool
+IsStructurallyEquivalentLambdas(StructuralEquivalenceContext ,
+CXXRecordDecl *D1, CXXRecordDecl *D2) {
+  assert(D1->isLambda() && D2->isLambda() &&
+ "Must be called on lambda classes");
+  if (!IsStructurallyEquivalent(Context, D1->getLambdaCallOperator(),
+D2->getLambdaCallOperator()))
+return false;
+
+  return true;
+}
+
 /// Determine structural equivalence of two records.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext ,
  RecordDecl *D1, RecordDecl *D2) {
@@ -1166,6 +1179,13 @@
 D1CXX->getASTContext().getExternalSource()->CompleteType(D1CXX);
   }
 
+  if (D1CXX->isLambda() != D2CXX->isLambda())
+return false;
+  if (D1CXX->isLambda()) {
+if (!IsStructurallyEquivalentLambdas(Context, D1CXX, D2CXX))
+  return false;
+  }
+
   if (D1CXX->getNumBases() != D2CXX->getNumBases()) {
 if (Context.Complain) {
   Context.Diag2(D2->getLocation(),
Index: cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
===
--- cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
+++ cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
@@ -797,6 +797,58 @@
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+struct StructuralEquivalenceLambdaTest : StructuralEquivalenceTest {};
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithDifferentMethods) {
+  // Get the LambdaExprs, unfortunately we can't match directly the underlying
+  // implicit CXXRecordDecl of the Lambda classes.
+  auto t = makeDecls(
+  "void f() { auto L0 = [](int){}; }",
+  "void f() { auto L1 = [](){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_FALSE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithEqMethods) {
+  auto t = makeDecls(
+  "void f() { auto L0 = [](int){}; }",
+  "void f() { auto L1 = [](int){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_TRUE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithDifferentFields) {
+  auto t = makeDecls(
+  "void f() { char* X; auto L0 = [X](){}; }",
+  "void f() { float X; auto L1 = [X](){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_FALSE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithEqFields) {
+  auto t = makeDecls(
+  "void f() { float X; auto L0 = [X](){}; }",
+  "void f() { float X; auto L1 = [X](){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_TRUE(testStructuralMatch(L0, L1));
+}
+
 TEST_F(StructuralEquivalenceTest, CompareSameDeclWithMultiple) {
   auto t = makeNamedDecls(
   "struct A{ }; struct B{ }; void foo(A a, A b);",
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -5122,6 +5122,22 @@
   EXPECT_EQ(ToLSize, FromLSize);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, LambdaInGlobalScope) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  auto l1 = [](unsigned lp) { return 1; };
+  auto l2 = [](int lp) { return 2; };
+  int f(int p) {
+return l1(p) + l2(p);
+  }
+  )",
+  Lang_CXX11, "input0.cc");
+  FunctionDecl *FromF = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("f")));
+  

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

@shafik I've been looking for any lldb regression in our Mac machine, could not 
find any. Now I am looking at 
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64075



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

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[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.

Hello Gabor,
This patch looks good to me.
Regarding the related patch: can we use getLambdaManglingNumber() for the 
comparison?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64075



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

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D64075#1572676 , @a_sidorin wrote:

> Hi Gabor,
>  The patch looks good, but it looks to me that it has a relation to 
> https://reviews.llvm.org/D64078 that is kind of questionable to me. Let's 
> delay landing this patch until the fix direction is clear.


Hi Alexey,

I have added a test case, which demonstrates the problem. When we import 
lambdas in a global/namespace scope then we crash. Let's try to import `f` from 
below:

  auto l1 = [](unsigned lp) { return 1; };
  auto l2 = [](int lp) { return 2; };
  int f(int p) {
return l1(p) + l2(p);
  }

Now during the import of the second lambda expression we crash because during 
the import of the second lambda class we find the existing lambda class and we 
just merge into that the new `operator()` and this results having two 
`operator()`s in the lambda class:

  ASTTests: ../../git/llvm-project/clang/lib/AST/DeclCXX.cpp:1392: 
clang::CXXMethodDecl *clang::CXXRecordDecl::getLambdaCallOperator() const: 
Assertion `allLookupResultsAreTheSame(Calls) && "More than one lambda call 
operator!"' failed.
#0 0x7f4d2b838d39 llvm::sys::PrintStackTrace(llvm::raw_ostream&) 
/home/egbomrt/WORK/llvm5/build/debug/../../git/llvm-project/llvm/lib/Support/Unix/Signals.inc:494:11
#1 0x7f4d2b838ee9 PrintStackTraceSignalHandler(void*) 
/home/egbomrt/WORK/llvm5/build/debug/../../git/llvm-project/llvm/lib/Support/Unix/Signals.inc:558:1
#2 0x7f4d2b8377d6 llvm::sys::RunSignalHandlers() 
/home/egbomrt/WORK/llvm5/build/debug/../../git/llvm-project/llvm/lib/Support/Signals.cpp:67:5
#3 0x7f4d2b83955b SignalHandler(int) 
/home/egbomrt/WORK/llvm5/build/debug/../../git/llvm-project/llvm/lib/Support/Unix/Signals.inc:357:1
#4 0x7f4d2b351390 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x11390)
#5 0x7f4d283d6428 gsignal 
/build/glibc-LK5gWL/glibc-2.23/signal/../sysdeps/unix/sysv/linux/raise.c:54:0
#6 0x7f4d283d802a abort 
/build/glibc-LK5gWL/glibc-2.23/stdlib/abort.c:91:0
#7 0x7f4d283cebd7 __assert_fail_base 
/build/glibc-LK5gWL/glibc-2.23/assert/assert.c:92:0
#8 0x7f4d283cec82 (/lib/x86_64-linux-gnu/libc.so.6+0x2dc82)
#9 0x7f4d2ab94b00 clang::CXXRecordDecl::getLambdaCallOperator() const 
/tmp/../../git/llvm-project/clang/lib/AST/DeclCXX.cpp:0:107
   #10 0x7f4d2acd3752 clang::LambdaExpr::getCallOperator() const 
/home/egbomrt/WORK/llvm5/build/debug/../../git/llvm-project/clang/lib/AST/ExprCXX.cpp:1205:3
   #11 0x7f4d2acd36c3 clang::LambdaExpr::LambdaExpr(clang::QualType, 
clang::SourceRange, clang::LambdaCaptureDefault, clang::SourceLocation, 
llvm::ArrayRef, bool, bool, llvm::ArrayRef, 
clang::SourceLocation, bool) 
/home/egbomrt/WORK/llvm5/build/debug/../../git/llvm-project/clang/lib/AST/ExprCXX.cpp:1128:34
   #12 0x7f4d2acd394f clang::LambdaExpr::Create(clang::ASTContext const&, 
clang::CXXRecordDecl*, clang::SourceRange, clang::LambdaCaptureDefault, 
clang::SourceLocation, llvm::ArrayRef, bool, bool, 
llvm::ArrayRef, clang::SourceLocation, bool) 
/home/egbomrt/WORK/llvm5/build/debug/../../git/llvm-project/clang/lib/AST/ExprCXX.cpp:1143:10
   #13 0x7f4d2a90b078 
clang::ASTNodeImporter::VisitLambdaExpr(clang::LambdaExpr*) 
/tmp/../../git/llvm-project/clang/lib/AST/ASTImporter.cpp:7461:10

Now if we slightly change the test code to make the lambdas op() have the very 
same signature then we end up with the problem we want to solve in D64078 
:

  auto l1 = [](int lp) { return 1; };
  auto l2 = [](int lp) { return 2; };
  int f(int p) {
return l1(p) + l2(p);
  }

There is no crash if the lambdas' decl context is a function or method, this is 
because in that case we simply skip the lookup, in `VisitRecordDecl` we have

  if (!DC->isFunctionOrMethod()) {
// lookup is done here!
  }

An alternative solution might be to disable lookup completely if the decl in 
the "from" context is a lambda. However, that would have other problems: what 
if the lambda is defined in a header file and included in several TUs? (I think 
we'd have as many duplicates as many includes we have, but we could live with 
that maybe). Would you prefer this alternative solution?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64075



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

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 208727.
martong added a comment.
Herald added a reviewer: shafik.

- Add test which crashes in baseline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64075

Files:
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  clang/unittests/AST/StructuralEquivalenceTest.cpp

Index: clang/unittests/AST/StructuralEquivalenceTest.cpp
===
--- clang/unittests/AST/StructuralEquivalenceTest.cpp
+++ clang/unittests/AST/StructuralEquivalenceTest.cpp
@@ -797,6 +797,58 @@
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+struct StructuralEquivalenceLambdaTest : StructuralEquivalenceTest {};
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithDifferentMethods) {
+  // Get the LambdaExprs, unfortunately we can't match directly the underlying
+  // implicit CXXRecordDecl of the Lambda classes.
+  auto t = makeDecls(
+  "void f() { auto L0 = [](int){}; }",
+  "void f() { auto L1 = [](){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_FALSE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithEqMethods) {
+  auto t = makeDecls(
+  "void f() { auto L0 = [](int){}; }",
+  "void f() { auto L1 = [](int){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_TRUE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithDifferentFields) {
+  auto t = makeDecls(
+  "void f() { char* X; auto L0 = [X](){}; }",
+  "void f() { float X; auto L1 = [X](){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_FALSE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithEqFields) {
+  auto t = makeDecls(
+  "void f() { float X; auto L0 = [X](){}; }",
+  "void f() { float X; auto L1 = [X](){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_TRUE(testStructuralMatch(L0, L1));
+}
+
 TEST_F(StructuralEquivalenceTest, CompareSameDeclWithMultiple) {
   auto t = makeNamedDecls(
   "struct A{ }; struct B{ }; void foo(A a, A b);",
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5083,6 +5083,22 @@
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
 ::testing::Values(ArgVector()), );
 
+TEST_P(ASTImporterOptionSpecificTestBase, LambdaInGlobalScope) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  auto l1 = [](unsigned lp) { return 1; };
+  auto l2 = [](int lp) { return 2; };
+  int f(int p) {
+return l1(p) + l2(p);
+  }
+  )",
+  Lang_CXX11, "input0.cc");
+  FunctionDecl *FromF = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("f")));
+  FunctionDecl *ToF = Import(FromF, Lang_CXX11);
+  EXPECT_TRUE(ToF);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
 DefaultTestValuesForRunOptions, );
 
Index: clang/lib/AST/ASTStructuralEquivalence.cpp
===
--- clang/lib/AST/ASTStructuralEquivalence.cpp
+++ clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -1085,6 +1085,19 @@
   return true;
 }
 
+/// Determine structural equivalence of two lambda classes.
+static bool
+IsStructurallyEquivalentLambdas(StructuralEquivalenceContext ,
+CXXRecordDecl *D1, CXXRecordDecl *D2) {
+  assert(D1->isLambda() && D2->isLambda() &&
+ "Must be called on lambda classes");
+  if (!IsStructurallyEquivalent(Context, D1->getLambdaCallOperator(),
+D2->getLambdaCallOperator()))
+return false;
+
+  return true;
+}
+
 /// Determine structural equivalence of two records.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext ,
  RecordDecl *D1, RecordDecl *D2) {
@@ -1166,6 +1179,13 @@
 D1CXX->getASTContext().getExternalSource()->CompleteType(D1CXX);
   }
 
+  if (D1CXX->isLambda() != D2CXX->isLambda())
+return false;
+  if (D1CXX->isLambda()) {
+if (!IsStructurallyEquivalentLambdas(Context, D1CXX, D2CXX))
+  return false;
+  }
+
   if (D1CXX->getNumBases() != D2CXX->getNumBases()) {
 

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
The patch looks good, but it looks to me that it has a relation to 
https://reviews.llvm.org/D64078 that is kind of questionable to me. Let's delay 
landing this patch until the fix direction is clear.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64075



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

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[Gabor Marton via Phabricator via cfe-commits]

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

- Add check for isLambda()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64075

Files:
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/unittests/AST/StructuralEquivalenceTest.cpp

Index: clang/unittests/AST/StructuralEquivalenceTest.cpp
===
--- clang/unittests/AST/StructuralEquivalenceTest.cpp
+++ clang/unittests/AST/StructuralEquivalenceTest.cpp
@@ -797,6 +797,58 @@
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+struct StructuralEquivalenceLambdaTest : StructuralEquivalenceTest {};
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithDifferentMethods) {
+  // Get the LambdaExprs, unfortunately we can't match directly the underlying
+  // implicit CXXRecordDecl of the Lambda classes.
+  auto t = makeDecls(
+  "void f() { auto L0 = [](int){}; }",
+  "void f() { auto L1 = [](){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_FALSE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithEqMethods) {
+  auto t = makeDecls(
+  "void f() { auto L0 = [](int){}; }",
+  "void f() { auto L1 = [](int){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_TRUE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithDifferentFields) {
+  auto t = makeDecls(
+  "void f() { char* X; auto L0 = [X](){}; }",
+  "void f() { float X; auto L1 = [X](){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_FALSE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithEqFields) {
+  auto t = makeDecls(
+  "void f() { float X; auto L0 = [X](){}; }",
+  "void f() { float X; auto L1 = [X](){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_TRUE(testStructuralMatch(L0, L1));
+}
+
 TEST_F(StructuralEquivalenceTest, CompareSameDeclWithMultiple) {
   auto t = makeNamedDecls(
   "struct A{ }; struct B{ }; void foo(A a, A b);",
Index: clang/lib/AST/ASTStructuralEquivalence.cpp
===
--- clang/lib/AST/ASTStructuralEquivalence.cpp
+++ clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -1085,6 +1085,19 @@
   return true;
 }
 
+/// Determine structural equivalence of two lambda classes.
+static bool
+IsStructurallyEquivalentLambdas(StructuralEquivalenceContext ,
+CXXRecordDecl *D1, CXXRecordDecl *D2) {
+  assert(D1->isLambda() && D2->isLambda() &&
+ "Must be called on lambda classes");
+  if (!IsStructurallyEquivalent(Context, D1->getLambdaCallOperator(),
+D2->getLambdaCallOperator()))
+return false;
+
+  return true;
+}
+
 /// Determine structural equivalence of two records.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext ,
  RecordDecl *D1, RecordDecl *D2) {
@@ -1166,6 +1179,13 @@
 D1CXX->getASTContext().getExternalSource()->CompleteType(D1CXX);
   }
 
+  if (D1CXX->isLambda() != D2CXX->isLambda())
+return false;
+  if (D1CXX->isLambda()) {
+if (!IsStructurallyEquivalentLambdas(Context, D1CXX, D2CXX))
+  return false;
+  }
+
   if (D1CXX->getNumBases() != D2CXX->getNumBases()) {
 if (Context.Complain) {
   Context.Diag2(D2->getLocation(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1182
 
+  if (D1CXX->isLambda()) {
+if (!D2CXX->isLambda())

a_sidorin wrote:
> Should we return false if `D1CXX->isLambda() != D2CXX->isLambda()`?
Thanks, good catch, I have added it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64075



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

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Gabor,
This looks mostly good but I have a question inline.




Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1182
 
+  if (D1CXX->isLambda()) {
+if (!D2CXX->isLambda())

Should we return false if `D1CXX->isLambda() != D2CXX->isLambda()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64075



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

[PATCH] D64075: [ASTImporter] Fix structural eq of lambdas

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added a reviewer: a_sidorin.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a project: clang.

The structural equivalence check reported false eq between lambda classes
with different parameters in their call signature.
The solution is to check the methods for equality too in case of lambda
classes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64075

Files:
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/unittests/AST/StructuralEquivalenceTest.cpp

Index: clang/unittests/AST/StructuralEquivalenceTest.cpp
===
--- clang/unittests/AST/StructuralEquivalenceTest.cpp
+++ clang/unittests/AST/StructuralEquivalenceTest.cpp
@@ -797,6 +797,58 @@
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+struct StructuralEquivalenceLambdaTest : StructuralEquivalenceTest {};
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithDifferentMethods) {
+  // Get the LambdaExprs, unfortunately we can't match directly the underlying
+  // implicit CXXRecordDecl of the Lambda classes.
+  auto t = makeDecls(
+  "void f() { auto L0 = [](int){}; }",
+  "void f() { auto L1 = [](){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_FALSE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithEqMethods) {
+  auto t = makeDecls(
+  "void f() { auto L0 = [](int){}; }",
+  "void f() { auto L1 = [](int){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_TRUE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithDifferentFields) {
+  auto t = makeDecls(
+  "void f() { char* X; auto L0 = [X](){}; }",
+  "void f() { float X; auto L1 = [X](){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_FALSE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest, LambdaClassesWithEqFields) {
+  auto t = makeDecls(
+  "void f() { float X; auto L0 = [X](){}; }",
+  "void f() { float X; auto L1 = [X](){}; }",
+  Lang_CXX11,
+  lambdaExpr(),
+  lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_TRUE(testStructuralMatch(L0, L1));
+}
+
 TEST_F(StructuralEquivalenceTest, CompareSameDeclWithMultiple) {
   auto t = makeNamedDecls(
   "struct A{ }; struct B{ }; void foo(A a, A b);",
Index: clang/lib/AST/ASTStructuralEquivalence.cpp
===
--- clang/lib/AST/ASTStructuralEquivalence.cpp
+++ clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -1085,6 +1085,19 @@
   return true;
 }
 
+/// Determine structural equivalence of two lambda classes.
+static bool
+IsStructurallyEquivalentLambdas(StructuralEquivalenceContext ,
+CXXRecordDecl *D1, CXXRecordDecl *D2) {
+  assert(D1->isLambda() && D2->isLambda() &&
+ "Must be called on lambda classes");
+  if (!IsStructurallyEquivalent(Context, D1->getLambdaCallOperator(),
+D2->getLambdaCallOperator()))
+return false;
+
+  return true;
+}
+
 /// Determine structural equivalence of two records.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext ,
  RecordDecl *D1, RecordDecl *D2) {
@@ -1166,6 +1179,13 @@
 D1CXX->getASTContext().getExternalSource()->CompleteType(D1CXX);
   }
 
+  if (D1CXX->isLambda()) {
+if (!D2CXX->isLambda())
+  return false;
+if (!IsStructurallyEquivalentLambdas(Context, D1CXX, D2CXX))
+  return false;
+  }
+
   if (D1CXX->getNumBases() != D2CXX->getNumBases()) {
 if (Context.Complain) {
   Context.Diag2(D2->getLocation(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits