[PATCH] D94293: [clangd] automatically index STL

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

(not sure if you were looking for comments yet, but i was just passing by and it was a small-ish patch, so couldn't resist :D)



================
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?
----------------
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?


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:527
 
+opt<bool> IndexSTL{
+    "index-stl",
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94293