[PATCH] D94293: [clangd] automatically index STL

[Sam McCall via Phabricator via cfe-commits]

sammccall 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.

Yeah, this seems at least annoying. There's some spectrum of:

- build indexes per language version and dynamically switch between them based on the file
- make this variable in some global way (e.g. a flag, or first file opened chooses the language version)
- language version is always (e.g.) c++14, hopefully it's better than nothing

Similar to the no-stdlib question and the multiple-languages question, this points to a separate index layer being a stronger design to allow dynamically swapping it (though worse can be better, too).



================
Comment at: clang-tools-extra/clangd/ClangdServer.h:161
+    // insertion
+    bool IndexSTL = false;
+
----------------
nit: "the standard library"

for two reasons:
 - "STL" is no longer the correct name, even for C++
 - languages other than C++ have standard libraries (C in particular) that we could also control with this flag


================
Comment at: clang-tools-extra/clangd/index/Background.cpp:451
+  //               indexer?
+  const Path STLHeaderPath = SourceRoot + "/.stl_header.h";
+  vlog("Indexing STL headers from {0}", STLHeaderPath);
----------------
separate from the issue of virtual vs real file, it's not clear to me why we *want* this file to be close to the project rather than in some completely anonymous location.

(I have a few guesses, but...)


================
Comment at: clang-tools-extra/clangd/index/Background.h:160
+    // TODO(kuhnel): Only index if enqueueing C++ file.
+    indexSTLHeaders(ChangedFiles);
   }
----------------
this can happen 0+ times (roughly it happens once every time an enumerable CDB is discovered).

Instead, we'd want it to happen exactly once, or possibly 0-1 times if we're going to be language-sensitive.


================
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.
Yeah, this is a good point.

Having this configurable would be valuable, but the useful granularity for such an option is per-file (Config) rather than a global clangd flag, as it's codebase-specific.

Unfortunately that doesn't really square with having it in the background index.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94293