ArcsinX wrote: > No, this is definitely the problem of this patch. The original example works > well before the patch and not working after the patch. You can't really say > an invariant of the problem exists too so it is not your problem.
I think clangd should reparse opened files, when dependent file changes (independent of is this include happens in preamble section or not). In this case clangd doesn't do this if include is not in the preamble section, so this looks like another problem. This patch introduces ability to avoid preamble build and I think this works as expected. But yes, we found some problem cases but these problems have different origin. Anyway, this is just an option, which can be disabled/enabled. > And also the variant you made is actually not by design of C++20 modules. > Inclusion in module purview will be warned. I don't see any compilation warnings. Anyway, I think there can be other legal ways to make includes to be in the middle of the file. > And even besides modules, again, this patch breaks the `CanReuse` > functionality of preamble. This is why I said we can't do so. As I said previously, it doesn't not, because we actually don't build the preamble, so we can say that we don't have the preamble and `CanReuse` functionality is out of scope here. This solution provides ability to fix crashes (at least for me, because https://github.com/llvm/llvm-project/pull/187432 is not enough and still I faced crashes at preamble index update with that patch) and just a try to help you to improve stability of c++20 modules support in clangd. If this is not acceptable for you because of the reasons above (which I believe have different original), maybe it's better to fix the problem in clang itself, as far as all we try to do here is to find a workaround for clang bugs. https://github.com/llvm/llvm-project/pull/189284 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
