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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
