ArcsinX 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. 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. https://github.com/llvm/llvm-project/pull/187432 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
