martong added inline comments.
================ Comment at: include/clang/AST/ASTImporter.h:317 + std::shared_ptr<ASTImporterSharedState> SharedState = nullptr, + ASTUnit *Unit = nullptr); ---------------- martong wrote: > 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. Note that if we have a ctor which takes FromUnit as a param then we won't need the assertions which check whether the Unit provides the same context or not. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64554/new/ https://reviews.llvm.org/D64554 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
