[PATCH] D39886: [ASTImporter] Fix wrong conflict detections for unnamed structures

Takafumi Kubota via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Nov 19 01:38:27 PST 2017


tk1012 updated this revision to Diff 123493.
tk1012 added a comment.
Herald added a subscriber: rnkovacs.

Hello,

I update the diff to solve the below thing.

> 1. Are import conflicts for anonymous structures resolved correctly?

Before I describe the updates, I want to detail the difference between unnamed structs and anonymous structs in clang.
According to the comment of RecordDecl::isAnonymousStructOrUnion in include/AST/Decl.h, 
there are unnamed structures that are not anonymous.
For example,

  union { int i; float f; } obj;
  struct { int a } A;

both of them are not anonymous but unnamed.

Originally, the anonymous structs/unions are checked by making sure that they occur in the same location.
However, the unnamed structures that are not anonymous are only checked by their name.
This results in the different unnamed structures are identified as the same one because their names are the null string.

So, this update makes the skip of the confliction check more strictly. 
It also adds a test for importing the anonymous unions

>   Are equal structures present in both TUs imported correctly, without duplication?

I think this is a more complicated problem because this does not depend on the structures are unnamed or not. 
For example, I try the below test and "make check-clang" fails due to "the Multiple declarations were found!".

  TEST(ImportDecl, ImportSameRecordDecl) {
    MatchVerifier<Decl> Verifier;
    EXPECT_TRUE(
          testImport(
            "struct A {"
            " int a;"
            "};",
            Lang_C,
            "struct A {"
            " int a;"
            "};",
            Lang_C, Verifier,
            recordDecl()));
  }

struct A is not anonymous and also not unnamed.

To avoid this, in my application, I check the same structure already exists in the "To" ASTContext before the import.
If the same struct is found, I map the "From" RecordDecl to the "To" one in advance.
However, I'm not sure I should request the review of this solution. ( I mean, should ASTImporter support this ?)
Even if I should, I think this feature must be in a different patch.

Best regards


https://reviews.llvm.org/D39886

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp


Index: unittests/AST/ASTImporterTest.cpp
===================================================================
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -485,6 +485,65 @@
                                        has(atomicType()))))))))));
 }
 
+TEST(ImportDecl, ImportUnnamedRecordDecl) {
+  MatchVerifier<Decl> Verifier;
+  EXPECT_TRUE(
+        testImport(
+          "void declToImport() {"
+          "  struct Root {"
+          "    struct { int a; } A;"
+          "    struct { float b; } B;"
+          "  } root;"
+          "}",
+          Lang_C, "", Lang_C, Verifier,
+          functionDecl(
+            hasBody(
+              compoundStmt(
+                has(
+                  declStmt(
+                    has(
+                      recordDecl(
+                        has(
+                          recordDecl(
+                            has(
+                              fieldDecl(
+                                hasType(asString("int")))))),
+                        has(
+                          recordDecl(
+                            has(
+                              fieldDecl(
+                                hasType(asString("float"))))))
+                        )))))))));
+}
+
+TEST(ImportDecl, ImportAnonymousRecordDecl) {
+  MatchVerifier<Decl> Verifier;
+  EXPECT_TRUE(
+        testImport(
+          "void declToImport() {"
+          "  struct Root {"
+          "    union { char a;};"
+          "    union { int b;};"
+          "  } root;"
+          "}",
+          Lang_C, "", Lang_C, Verifier,
+          functionDecl(
+            hasBody(
+              compoundStmt(
+                has(
+                  declStmt(
+                    has(
+                      recordDecl(
+                        has(
+                          recordDecl(
+                            has(
+                              fieldDecl(hasType(asString("char")))))),
+                        has(
+                          recordDecl(
+                            has(
+                              fieldDecl(hasType(asString("int"))))))
+                        )))))))));
+}
 
 } // end namespace ast_matchers
 } // end namespace clang
Index: lib/AST/ASTImporter.cpp
===================================================================
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1631,7 +1631,12 @@
   // We may already have a record of the same name; try to find and match it.
   RecordDecl *AdoptDecl = nullptr;
   RecordDecl *PrevDecl = nullptr;
-  if (!DC->isFunctionOrMethod()) {
+
+  // There are unnamed structures that are not anonymous.
+  // They cause wrong conflict detections due to the null string name.
+  bool isNotAnonymousButUnnamed = (!D->isAnonymousStructOrUnion() && SearchName.getAsString() == "");
+
+  if (!DC->isFunctionOrMethod() && !isNotAnonymousButUnnamed) {
     SmallVector<NamedDecl *, 4> ConflictingDecls;
     SmallVector<NamedDecl *, 2> FoundDecls;
     DC->getRedeclContext()->localUncachedLookup(SearchName, FoundDecls);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D39886.123493.patch
Type: text/x-patch
Size: 3065 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171119/f92d6517/attachment.bin>


More information about the cfe-commits mailing list