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