[PATCH] D113118: [clang][AST] Check context of record in structural equivalence.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I very much like the code.
L1364 is uncovered according to my coverage results.



================
Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1360-1364
+    // Special case: We allow a struct defined in a function to be equivalent
+    // with a similar struct defined outside of a function.
+    if ((DC1->isFunctionOrMethod() && DC2->isTranslationUnit()) ||
+        (DC2->isFunctionOrMethod() && DC1->isTranslationUnit()))
+      return true;
----------------
Shouldn't you also check the name as well?


================
Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1379
+
+    DC1 = DC1->getParent()->getNonTransparentContext();
+    DC2 = DC2->getParent()->getNonTransparentContext();
----------------
So, the previous `DC1->isTranslationUnit()` guards the `DC1->getParent()` to never be `NULL`; same for `DC2`.
Pretty neat.


================
Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:982-988
+TEST_F(StructuralEquivalenceRecordContextTest, NamespaceNamedVsInline) {
+  auto Decls = makeDecls<CXXRecordDecl>(
+      "namespace A { namespace A { class X; } }",
+      "namespace A { inline namespace A { class X; } }", Lang_CXX17,
+      cxxRecordDecl());
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
----------------
The outer-most namespace could also be //inline//.
I'd love to see a test about that as well.


================
Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1004-1009
+TEST_F(StructuralEquivalenceRecordContextTest, TransparentContextNE) {
+  auto Decls = makeDecls<CXXRecordDecl>("extern \"C\" { class X; }",
+                                        "namespace { class X; }", Lang_CXX17,
+                                        cxxRecordDecl());
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
----------------
`extern "C"` might be nested within other namespaces.
Probably worth testing that as well.


================
Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1025-1032
+TEST_F(StructuralEquivalenceTest, StructDefinitionInPrototype) {
+  auto Decls = makeDecls<ParmVarDecl>(
+      "struct Param { int a; }; void f(struct Param *p);",
+      "void f(struct Param { int a; } *p);", Lang_C89,
+      parmVarDecl(hasName("p")));
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
----------------
Please, also test if the name of the structs is mismatched.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113118