ChuanqiXu9 wrote: > > There are 2 things in the patch. One is to generate the BMI and the object > > file in one phase (phase here means preprocess, precompile, compile, ...). > > This is the main point of the patch - to do this efficiently.
Got it. The we can be more focused. > > > But after we introduced thin BMI, it looks inefficient to write the AST > > twice. So it is on my TODO list after we land the thin BMI patch. BTW, I > > think we should do thin in CodeGen action instead of hacking on > > WrappedASTConsumer. > > I am curious as to why you think that the multiplex AST consumer is a hack - > it seems to be designed exactly for this purpose and existed already (i.e. > not part of this patch). It is not about multiplex AST consumer. It is about WrappedASTConsumer. It is designed for plugins. Also it is a private member function of FrontendAction, the base of frontend actions. I think we should perform new behaviors in sub-actions. It looks not good to perform semantical analysis in FrontendAction... Concretely, I think we need to do this in CodeGenAction. > > > And if we introduce the mechanism to produce BMI for `.cpp`, it implies > > that we need to maintain both paths. It is super embracing to me. > > We do not need two mechanisms, .cppm can take the same path as any other > suffix. Then it implies that we need to discard a bunch of existing codes handling `.cppm`. Otherwise we'll have two mechanisms. > > > > in the AST consumer on the BMI side doing suitable filtering to eliminate > > > the content that is not part of the interface, that is either not needed > > > (or in some cases positively unhelpful to consumers). > > > I believe we should do this in ASTWriters. > > I am strongly against doing more semantic work in the AST reader/writer; that > is just compounding existing layering violations that are already hurting us. Agreed in the higher level. But that requires us to implement at least new AST writers. > > > Also this should be part of thin BMI. > > I am not sure what you mean here - the full AST is required for code-gen - we > can only thin AST either on a separate path (as in this patch) or as a > separate step. I mean it should be successors of https://github.com/llvm/llvm-project/pull/71622. Concretely, now we reduce the function definition in https://github.com/llvm/llvm-project/pull/71622/files#diff-125f472e690aa3d973bc42aa3c5d580226c5c47661551aca2889f960681aa64dR321. https://github.com/llvm/llvm-project/pull/71773 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
