[PATCH] D94293: [clangd] automatically index STL

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D94293#2486468 <https://reviews.llvm.org/D94293#2486468>, @njames93 wrote:

> How does this approach work with differing language standards. For example with an old implementation designed for c++14, `string_view` header won't exist. If the implementation is designed to work with c++17, including that header in c++14 mode will probably be ifdef... Error.

As I mentioned in my comments for putting this behind a flag, in such scenarios we should just hit some "missing includes" while indexing the phantom file, so all should be fine.



================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:527
 
+opt<bool> IndexSTL{
+    "index-stl",
----------------
kuhnel wrote:
> 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.
I see. It still feels like those developers would be less inclined to accept autocompletion of `std::vector` compared to `xyz::vector` and also our ranking should be doing a good job of promoting `xyz::vector` due to its high number of usage, compared to `std::vector`.

I am mostly unhappy about the flag as it needs propagation to many components. It might be better to have it as a config option, which requires a lot less plumbing (at least for bg-index), if you think even the down-ranking of `std` symbols wouldn't be a good experience.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94293