martong marked 4 inline comments as done. martong added a comment. @xazax.hun Hey Gabor, thanks a lot for your time and effort for reviewing this patch set! (And of course thank you for you too @steakhal, but you've already seen most of it.)
In D123685#3458329 <https://reviews.llvm.org/D123685#3458329>, @steakhal wrote: > What happens if the import fails? Then `Import` returns with an `Error` object. > Or alternatively the `InitializeImportedDecl(FromD, ToD)` fails? That function can never fail, that is just setting a few bits: void InitializeImportedDecl(Decl *FromD, Decl *ToD) { ToD->IdentifierNamespace = FromD->IdentifierNamespace; if (FromD->isUsed()) ToD->setIsUsed(); if (FromD->isImplicit()) ToD->setImplicit(); } ================ Comment at: clang/include/clang/AST/ASTImporterSharedState.h:43 + /// Set of the newly created declarations. + llvm::DenseSet<Decl *> NewDecls; + ---------------- xazax.hun wrote: > ASTImporter already has something like `ImportedFromDecls`. Is that not > sufficient to check if a declaration is new? > > Is it possible that we may want the "degree" of the imported definition? > I.e., how many hops did we do to import it (is it imported as a result of > evaluating an imported call?). What we need here is a list of those declarations that have been **created** and linked into the destination TU by the ASTImporter. `ImportedFromDecls` is a mapping of declarations from the "source" context to the "destination" context. It might happen that we do not create a new declaration during the import, however, the mapping is still updated, so next time we can just simply get that from there (it's a cache). E.g. during the import of `foo` from `b.cpp` to `a.cpp` we are going to import `X` as well. But during the import of `X` we find the existing definition and then we just simply update the mapping to that. This happens all the time when we include the same header to two different translation units. ``` // a.cpp struct X {}; // b.cpp struct X {}; void foo(X); ``` > Is it possible that we may want the "degree" of the imported definition? > I.e., how many hops did we do to import it I am not sure how that would be useful, I mean how and for what could we use that information? ================ Comment at: clang/include/clang/AST/ASTImporterSharedState.h:69-75 llvm::Optional<ImportError> getImportDeclErrorIfAny(Decl *ToD) const { auto Pos = ImportErrors.find(ToD); if (Pos != ImportErrors.end()) return Pos->second; else return Optional<ImportError>(); } ---------------- steakhal wrote: > Why does this API accept only non-const pointers? This is legacy from the old times, internal AST functions all take non-const pointers. However, I agree, we could extend the API with overloads that take a `const`, but that should be orthogonal to this change. ================ Comment at: clang/include/clang/AST/ASTImporterSharedState.h:81 + + bool isNewDecl(const Decl *ToD) const { return NewDecls.count(ToD); } + ---------------- xazax.hun wrote: > I assume this would only be applicable for definitions, so I wonder whether > `IsNewDefinition()` would be more descriptive. Or maybe > `IsImportedDefinition`? In the context of CTU, you are right, we are interested in definitions only. However, it would be over-complication to do that distinction at the `ASTImporter` level, I think. ================ Comment at: clang/lib/AST/ASTImporter.cpp:248 Importer.RegisterImportedDecl(FromD, ToD); + Importer.SharedState->setNewDecl(ToD); InitializeImportedDecl(FromD, ToD); ---------------- xazax.hun wrote: > Should this be part of `Importer.RegisterImportedDecl`? No. `RegisterImportedDecl` is being overwritten in some of the descendant lldb classes. We want `setNewDecl` called no matter what. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123685/new/ https://reviews.llvm.org/D123685 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
