[PATCH] D116155: [clang][AST][ASTImporter] Set record to complete during import of its members.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.


================
Comment at: clang/lib/AST/ASTImporter.cpp:2021
+  // fields imported) at that time without multiple AST import passes.
+  To->setCompleteDefinition(true);
   // Complete the definition even if error is returned.
----------------
shafik wrote:
> So `DefinitionCompleterScopeExit` will run `To->setCompleteDefinition(false);`  after this function exits but this will be in effect during the import the base classes. I don't see how the tests you added hit that code.
Should the test `CompleteRecordBeforeImporting` not do the  import in minimal mode? The comment says minimal but in `ASTImporter` minimal mode is not set. The test will fail because this added line. But I think it should work to call the `To->setCompleteDefinition` here only in non-minimal mode.


================
Comment at: clang/lib/AST/ASTImporter.cpp:2088
+    // Set to false here to force "completion" of the record.
+    To->setCompleteDefinition(false);
     if (Error Err = ImportDeclContext(From, /*ForceImport=*/true))
----------------
My fix was not correct. This line was added to make a test `CompleteRecordBeforeImporting` not fail but it makes the original fix not work. Something other is needed.


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:7485
+        static void foo(A x) {
+          (void)&"text"[x.idx];
+        }
----------------
shafik wrote:
> The member function body should be considered `complete-class context` so the correct thing to do would be have all the fields laid out by this point.
My initial plan was to import first all fields, then everything else. But it is possible that a field has reference to the same record before the import of all fields finishes (like in the second test) so this can not work in all cases (and it caused other test failures).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116155