[PATCH] D123685: [clang][ASTImporter] Add isNewDecl

[Gabor Marton via Phabricator via cfe-commits]

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