[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. I sent the next patch at: https://github.com/llvm/llvm-project/pull/66462 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 ___ cfe-commits mailing list

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. Got it. Thanks for your explanation. I can continue my experiment : ) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 ___ cfe-commits mailing list

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

sammccall added a comment. In D153114#4630318 , @ChuanqiXu wrote: > @sammccall @nridge while I am looking for the initial support for modules in > clangd, I failed to find the mechanism to update files after I update a > header file. > > e.g., when I

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. In D153114#4630408 , @nridge wrote: > In D153114#4630318 , @ChuanqiXu > wrote: > >> However, I can't search the caller of `reparseOpenFilesIfNeeded` which >> semantics matches the

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

nridge added a comment. In D153114#4630318 , @ChuanqiXu wrote: > However, I can't search the caller of `reparseOpenFilesIfNeeded` which > semantics matches the behavior. The two callers of > `reparseOpenFilesIfNeeded` I found are >

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. @sammccall @nridge while I am looking for the initial support for modules in clangd, I failed to find the mechanism to update files after I update a header file. e.g., when I am opening the following file: // a.cc #include "a.h" ... and there is a concurrent

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

sammccall added a comment. Aside: I've been doing some investigation into how modules+clangd could work in our huge monorepo (specifically bazel + distributed build cluster). It looks feasible (with some serious effort) to get all BMI/index/etc data we need for transitive modules to be

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. @sammccall here is a question (or double check) about the intended initial version. This is the requirement for the initial version: > Don't attempt any cross-file or cross-version coordination: i.e. don't try to > reuse BMIs between different files, don't try to

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. In D153114#4603579 , @sammccall wrote: > In D153114#4602414 , @ChuanqiXu > wrote: > >>> Don't attempt any cross-file or cross-version coordination: i.e. don't try >>> to reuse BMIs

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

sammccall added a comment. In D153114#4604438 , @nridge wrote: > In D153114#4603579 , @sammccall > wrote: > >> - to identify what module names we must have PCMs for in order to build a >> given TU (either an

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

nridge added a comment. In D153114#4603579 , @sammccall wrote: > Dep scanning - roles > > > IIUC we do this for two reasons: > > - to identify what module names we must have PCMs for in order to build a > given TU (either an open

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

sammccall added a comment. In D153114#4602414 , @ChuanqiXu wrote: >> Don't attempt any cross-file or cross-version coordination: i.e. don't try >> to reuse BMIs between different files, don't try to reuse BMIs between >> (preamble) reparses of the same

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. @sammccall Hi, Sam. Thanks for your high-quality comments! It is valuable. All the low-level inline comments are helpful. But I didn't reply them for the suggested direction in the higher level comments. I'll repeat your suggestion in my mind again to avoid any

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

sammccall added a comment. Sorry for the long radio silence here. There's a lot to chew on, and I put it off too long. Thanks for your patience! I agree we should get experimental modules support landed in some form on the LLVM 18 timeline. It's fine if this doesn't have extension points for

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ilya-biryukov added a comment. In D153114#4571703 , @ChuanqiXu wrote: > For example, may it be a reasonable expectation that we can have named > modules support in clangd in clang18? Clang18 will be released in the first > week of March 2024. So that's

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. Got it. The explanation makes sense. A well designed and scaling solution is what I (and probably every one) want. Then from the expectation, the difference between supporting header modules and C++20 named modules from the **user** side may be: - For supporting

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ilya-biryukov added a comment. In D153114#4569591 , @ChuanqiXu wrote: > BTW, I have a question about supporting header modules (including clang > header modules and C++20 header units) in static analysing tools (including > clangd), "why can't we

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. BTW, I have a question about supporting header modules (including clang header modules and C++20 header units) in static analysing tools (including clangd), "why can't we fallback to include the corresponding headers simply?". So I still feel it is not a problem to

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. Got it. Being patience is not bad and it is good enough to know that we are moving forward : ) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 ___ cfe-commits mailing

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Hi @ChuanqiXu, Sam is on vacation now, but we are in the same team and I am responding on behalf of the team. First, sorry for not getting to this review earlier. The

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. @sammccall gentle ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. @sammccall gentle ping~ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

nridge resigned from this revision. nridge added a comment. I'm sorry, I feel like I don't have a good enough level of insight into the requirements to usefully critique this patch, nor the bandwidth to take a detailed look through the implementation right now. I think it's best for me to

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. @sammccall @nridge gentle ping~ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. @sammccall @nridge gentle ping~ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

Destroyerrrocket added inline comments. Comment at: clang-tools-extra/clangd/ModulesManager.cpp:413-414 + else +WaitingCallables[Filename.str()].push_back( +{std::move(ReadyCallback), std::move(ReadyCallback)}); +} ChuanqiXu wrote: >

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added inline comments. Comment at: clang-tools-extra/clangd/ModulesManager.cpp:413-414 + else +WaitingCallables[Filename.str()].push_back( +{std::move(ReadyCallback), std::move(ReadyCallback)}); +} Destroyerrrocket wrote: > This is a bug;

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu updated this revision to Diff 532502. ChuanqiXu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153114/new/ https://reviews.llvm.org/D153114 Files: clang-tools-extra/clangd/CMakeLists.txt clang-tools-extra/clangd/ClangdLSPServer.cpp

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

Destroyerrrocket requested changes to this revision. Destroyerrrocket added inline comments. This revision now requires changes to proceed. Comment at: clang-tools-extra/clangd/ModulesManager.cpp:413-414 + else +WaitingCallables[Filename.str()].push_back( +

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ChuanqiXu added a comment. > That said, there are two large issues that I think should be addressed in the > design (though not necessarily *implemented* now). Yeah, totally agreed. Design is pretty important especially in open source softwares. I'm pretty open to the suggestions. --- >

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

nridge added a comment. > The major problem I see now is that we can't handle third party modules. > Third party modules refer to the modules whose source codes are not in > current project. The users are still able to see the hint from clangd if the > BMI of the third party modules are built

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

sammccall added a comment. (Sorry, hit send too soon) I suspect the answer for header modules is that we can study this patch and understand what the equivalents of graph nodes/deps/names/scanning look like for explicit header modules, and understand that we'll be able to abstractify some

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

sammccall added a comment. Thanks for putting this together, I'm going to study it carefully and try it out! That said, there are two large issues that I think should be addressed in the design (though not necessarily *implemented* now). I'll be upfront: these are things without which