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

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor!
I haven't find the import sequence examples we try to fix these ways in any of the three patches these change consists of. Could you please provide some (or point if I missed them)?



================
Comment at: clang/include/clang/AST/ASTImporterSharedState.h:46
+public:
+  ASTImporterSharedState() {}
+
----------------
`= default?`


================
Comment at: clang/lib/AST/ASTImporter.cpp:7724
 void ASTImporter::AddToLookupTable(Decl *ToD) {
-  if (LookupTable)
+  if (SharedState->getLookupTable())
     if (auto *ToND = dyn_cast<NamedDecl>(ToD))
----------------
```
if (auto* LookupTable = ..._)
  ...
    LookupTable->add();
```
looks a bit cleaner to me.


================
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))
----------------
I'm not sure if it is safe to compare declarations from different TUs by pointers because they belong to different allocators.


================
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);
----------------
getImportDeclErrorIfAny() is usually called for FromD, not for ToD. Is it intentional?


================
Comment at: clang/lib/AST/ASTImporter.cpp:7867
       if (PosF != ImportedFromDecls.end()) {
-        if (LookupTable)
+        if (SharedState->getLookupTable())
           if (auto *ToND = dyn_cast<NamedDecl>(ToD))
----------------
I think we can encapsulate these conditions into `SharedState::[add/remove]Decl[To/From]Lookup methods.


================
Comment at: clang/lib/AST/ASTImporter.cpp:7924
+  if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))
+    return make_error<ImportError>(*Error);
+
----------------
I don' think that an import failure from another TU means that the import from the current TU will fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62376