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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
