[PATCH] D94293: [clangd] automatically index STL

[Christian Kühnel via Phabricator via cfe-commits]

kuhnel added a comment.

Yes, I was looking for feedback on this one as I wasn't happy with my design.



================
Comment at: clang-tools-extra/clangd/index/Background.cpp:449
+  // TODO(kuhnel): is the a better place to store this file?
+  // TODO(kuhnel): do we need a file at all, can we just pass a string to the
+  //               indexer?
----------------
kadircet wrote:
> In theory BackgroundIndex only reads headers from the FS, so we can provide an in-memory buffer as the main file itself. `prepareCompilerInstance` in `Compiler.h` (and used by `BackgroundIndex::index` does that exactly).
> 
> It might be better to just have a separate endpoint in BackgroundIndex (as `indexSTLHeaders`) that is invoked once at construction time that enqueues indexing of this phantom file.
> 
> WDYT?
Agree to both. I'll take a look.


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:527
 
+opt<bool> IndexSTL{
+    "index-stl",
----------------
kadircet wrote:
> do we really need to hide this behind a flag ? it sounds like a quite useful feature to me with minimal risk of regressions and unlikely to make anyone upset. in the end we are not doing anything heuristically and just throwing clang on some STL headers, that'll probably be included within the TUs eventually.
> 
> there's always the risk of crashing while parsing some STL headers, but i don't think it is any different than user just including the header manually.
> 
> the biggest problem i can see is people using custom STL headers, but hopefully compile flags interpolation logic should be able to infer the relevant location for those. and in the cases it fails we either index the default STL and suggest people some symbols their implementation might lack, or fail to find STL at all and print some logs for the missing includes.
Back when I was developing embedded software, we couldn't use the STL for various reasons (object size, no exceptions, no dynamic memory, ...) and had our own, proprietary base library. In such a scenario it would be annoying if clangd would be proposing things I could not use in the project. 

So we should have a way for users to disable this. I'm fine if we switch it on by default and offer a `--disable-stl-index` flag instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94293/new/

https://reviews.llvm.org/D94293