jkorous marked an inline comment as done. jkorous added a comment. In D58418#1431765 <https://reviews.llvm.org/D58418#1431765>, @thakis wrote:
> In D58418#1431399 <https://reviews.llvm.org/D58418#1431399>, @jkorous wrote: > > > In D58418#1430630 <https://reviews.llvm.org/D58418#1430630>, @thakis wrote: > > > > > In D58418#1430490 <https://reviews.llvm.org/D58418#1430490>, @jkorous > > > wrote: > > > > > > > In D58418#1430160 <https://reviews.llvm.org/D58418#1430160>, @thakis > > > > wrote: > > > > > > > > > Why is this needed for index-while-building? My mental model for > > > > > index-while-building is that that streams out build index metadata as > > > > > part of the regular compile. Why does that require watching > > > > > directories? > > > > > > > > > > > > You're right that this isn't necessary for the indexing phase. But we > > > > also provide API so clients can consume the index. This functionality > > > > is used for getting notifications about index data changes. > > > > > > > > You can see it for example here: > > > > > > > > https://github.com/apple/swift-clang/blob/stable/lib/IndexDataStore/IndexDataStore.cpp#L111 > > > > > > > > > Is that code going to live in clang? This seems more like a tool built on > > > top of the compiler rather than something core to the compiler itself > > > (like the actual index-while-building feature). Maybe this could be in > > > clang-tools-extra? > > > > > > It actually is part of the feature as the serialized format of the index > > isn't meant as a stable interface, that's what the API is for. > > DirectoryWatcher isn't a tool, it's just part of implementation of the > > IndexStore API. > > > Maybe I misunderstand what the client of the IndexStore API is. That's not > code that will be in the clang binary, right? No, it won't. Currently the client using this API is our indexing service. https://github.com/apple/sourcekit-lsp In the future clangd might become another client. In theory we could have the index producing part in clang and the index consuming part (IndexStore) somewhere else (clang-tools-extra?) and use functionality that also lives somewhere else (llvm?) but reasons I'd rather not do it *NOW* are: 1. We'd have to expose the interface between them (the binary file format) which has been just an implementation detail so far without any intention to keep it stable. 2. From the general perspective - although I am upstreaming a fully developed feature (roughly 10kLOC) it is apparent that I am going to rewrite a significant part of the code based on the feedback from reviews. This patch is #2 out of approximately 10-15 patches total. Since it's probable that the design will change in upcoming reviews I'd prefer to discuss this kind of questions after a significant part of the whole design has been through the review. 3. For any code that we would move up the tree (to llvm repo) I'd like to have a clear use-case other than index-while-building first. Designing generic APIs is hard/impossible without known specific use-cases (I think the recommended minimum is 3). The most important word is "now". I am totally happy to discuss this and move parts somewhere else if it seems reasonable in the future. Does that make sense? ================ Comment at: clang/include/clang/DirectoryWatcher/DirectoryWatcher.h:27 +/// be invoked even if the directory is empty. +class DirectoryWatcher { +public: ---------------- gribozavr wrote: > I feel like since there's nothing Clang-specific about it, it should go into > libSupport in LLVM, what do you think? This has been brought up before. I prefer to leave it here for now since it's not used anywhere else. I'd only move it to llvm/Support once we have another use-case as that would mean specific requirements for the interface. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58418/new/ https://reviews.llvm.org/D58418 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
