[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st

[Gabor Marton via Phabricator via cfe-commits]

martong added a reviewer: balazske.
martong marked 6 inline comments as done.
martong added inline comments.


================
Comment at: clang/lib/AST/ASTImporter.cpp:7830
   if (ToD) {
+    // Already imported (possibly from another TU) and with an error.
+    if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))
----------------
a_sidorin wrote:
> I'm not sure if it is safe to compare declarations from different TUs by pointers because they belong to different allocators.
In the `SharedState` we keep pointers only from the "to" context.


================
Comment at: clang/lib/AST/ASTImporter.cpp:7831
+    // Already imported (possibly from another TU) and with an error.
+    if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))
+      return make_error<ImportError>(*Error);
----------------
a_sidorin wrote:
> getImportDeclErrorIfAny() is usually called for FromD, not for ToD. Is it intentional?
`ASTImporter::getImportDeclErrorIfAny()` is different from `ASTImporterSharedState::getImportDeclErrorIfAny()`.

The former keeps track of the erroneous decls from the "from" context.

In the latter we keep track of those decls (and their error) which are in the "to" context.
The "to" context is common for all ASTImporter objects in the cross translation unit context.
They share the same ASTImporterLookupTable object too. Thus the name ASTImporterSharedState.


================
Comment at: clang/lib/AST/ASTImporter.cpp:7924
+  if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))
+    return make_error<ImportError>(*Error);
+
----------------
a_sidorin wrote:
> I don' think that an import failure from another TU means that the import from the current TU will fail.
It should. At least if we map the newly imported decl to an existing but already messed up decl in the "to" context. Please refer to the added test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62376