ChuanqiXu9 wrote: > > And there are more clangd crash reports, which I suspect is due to mixing > > PCH and clang named modules. Since all these reports said the project can > > be built with clang but not clangd. And from implementors point of view, I > > don't think we can fix this in clang in any time soon. > > So, this patch is not just a fix for #181770, but also a stability > improvement.
yes > > > For the first point, as the maintainer of modules in clang, I deeply > > concerns the usablity of mixing PCH and named modules in clang. > > But with this patch we build preamble with module import inside This is the key point. Currently, C++20 modules' import is not considered as part of the preamble. > > > And from my point of view, from clang and clangd's perspective, including > > technical point and human resources and community ecosystem, this is the > > best solution we can have right now > > So, if now we are talking about clangd stability with modules support enabled > (but not about wrong diagnostics appearing), I think it's ok just not to > build a preamble in such cases (and maybe it's ok not to build preambles at > all if experimental modules support is enabled?). With this patch the > behavior is a bit weird: > > * we are still building preamble with module import inside, so mixing PCH > and C++20 modules, which is not good, as you said. > > * we drop preamble after that > > > You mentioned in this patch description, skipping preamble build is something > slightly complex, but maybe we can put a bit effort to do this, because it > looks like a correct direction. My thought is, it is fine if we can workaround the problem by the small patch, the meaningful change in ParsedAST.cpp is less than 10 lines. So it won't hurt maintainability and also it won't hurt anyone. So it is fine. And if we tried to skip preamble building, we may spend roughly 50~100 lines of code in my previous experience. And the longer the patch is, the risk may be higher and the cost for us to understand it goes high too. **Code is a debt**. And most important is, it is still a workaround after all. So my thought is, the simple, easy to understand and innocent current workaround patch is better than the more complex one which may cost more time from developers and reviewers, and also end users. I feel this is benefit for end users too as they need the support to be usable first and they don't need the peak performance right now. We can try to do this in future but I don't think we have to make them all together right now. I think I can understand your thought. I tried to do that but finally I delete my patch. Simpler is better. https://github.com/llvm/llvm-project/pull/187432 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
