[PATCH] D64554: [CrossTU] Add a function to retrieve original source location.

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: include/clang/AST/ASTImporter.h:317
+                std::shared_ptr<ASTImporterSharedState> SharedState = nullptr,
+                ASTUnit *Unit = nullptr);
 
----------------
balazske wrote:
> balazske wrote:
> > martong wrote:
> > > What if we provided an additional constructor where we take over the ASTUnits instead of the ASTContexts?
> > > Then we would not need to pass the FileManagers neither.
> > > ```
> > >     ASTImporter(ASTUnit &ToUnit, 
> > >                 ASTUnit &FromUnit,
> > >                 bool MinimalImport,
> > >                 std::shared_ptr<ASTImporterSharedState> SharedState = nullptr,
> > > ```
> > Is the `SharedState==nullptr` case only for LLDB? If yes then it is possible to have a "LLDB" constructor when no shared state and no ASTUnit is needed. And another for CTU case when a From and To ASTUnit is specified and a shared state (theoretically minimal can be true in any case but probably only true for LLDB?).
> It is not trivial to make and use this new kind of constructor (with ToUnit), there is no ToUnit in the CrossTU context. Is it OK to have the original constructor, or one with `FromUnit` (but `ToContext` and `ToFileManager`)?
> Is the SharedState==nullptr case only for LLDB?
Yes.

> Is it OK to have the original constructor, or one with FromUnit (but ToContext and ToFileManager)?

I think it is OK to have a new ctor with FromUnit (but ToContext and ToFileManager). Because ASTUnit is just a utility class for loading an ASTContext from an AST file. So, one of the "to" or the "from" context could be initialized by an ASTUnit.

In the future the API of the ASTImporter could be extended with such a ctor which takes two ASTUnits, also we could add the counterpart ctor where we have ToUnit, FromContext and FromFileManager as params.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64554