ChuanqiXu9 wrote: > > 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. > > I have some doubts here about "won't hurt anyone". Without this patch for > scenario when we have a module import in TU body (but not inside > `#include`s), everything works well.
No, there are countless reports about clangd crash with modules. And a lot of users don't open issue reports and they just say "clangd supports modules not usable". This is very important that, currently modules with clangd are not working good. Currently clangd's support for modules are not working well. > But with this patch we build preamble and silently skip it, forcing AST build > from the whole file (with all `#include`s) at every file modification, which > is a typical development scenario (when we don't modify headers section > frequently). > > So, here we are trying to solve clangd stability and avoid some crashes > there, but these crashes are also happens inside clang, I guess. > > But, as far as modules support is experimental feature in clangd, I think > it's ok not to build preamble at all when we have modules import in TU (or > maybe for any TU if experimental modules support is enabled). But building > preamble and silently dropping it just because it less code to write doesn't > look as a right approach. This really sounds a bit strange to me: we are > fixing one problem by introducing another one, taking into account that > original problem lives in clang (not clangd), but it's not convenient to fix > it in clang right now, and introduced problem is a clangd problem. I can understand your concern from clangd's perspective. But from my point view for the all aspects: clang's implementation, clangd's implementation, developer's resources and user experiences, this is the best solution we can have right now. Users will have more usable clangd supports. Developer, reviewers and maintainers will save their time. The patch itself in ParsedAST is less than 10 lines. Easy to understand and won't affect users who don't use modules. And finally, I think, we have consensus on code level. And what we have gap is, the experience and feelings for users of modules community and the realility of lacking implementation resources. Although this is slightly strange to say in a code review, but I think you can trust me that I am keeping making things working in limited resources. https://github.com/llvm/llvm-project/pull/187432 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
