[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D58345#1401907 <https://reviews.llvm.org/D58345#1401907>, @sammccall wrote:

> In D58345#1401213 <https://reviews.llvm.org/D58345#1401213>, @hokein wrote:
>
> > In D58345#1401040 <https://reviews.llvm.org/D58345#1401040>, @ioeric wrote:
> >
> > > Looks great! Thanks for doing this.
> > >
> > > Could you also check in the tool that is used to generate the mapping? We need a way to update the mapping when cppreference is updated.
> >
> >
> > The tool is not ready yet, I'm still polishing it, but the results are good, I think this patch should not be blocked by the tool.
>
>
> Generated files aren't really source code, I agree with Eric we should check in the tool with this patch.


Added the tool in this patch, but I think a separate repository under GitHub's clangd org might be a more suitable place.

For simplicity, I re-scoped this patch -- only emits unique symbols. We have ideas to handle ambiguous symbols, will address them in a followup.



================
Comment at: clangd/StdSymbolMap.imp:410
+{ "std::gslice_array", { "<valarray>" } },
+{ "std::hardware_constructive_interference_size", { "<new>" } }, // C++11
+{ "std::hardware_destructive_interference_size", { "<new>" } }, // C++11
----------------
sammccall wrote:
> I'm not sure if the language comments are useful to human readers, who can just look this stuff up. If you think they might be useful to code, we could include them in  tablegen-style macros.
> 
> In any case, this is a C++17 symbol, so maybe something's wrong with the generator.
> 
> In any case, this is a C++17 symbol, so maybe something's wrong with the generator.

hmm, it is a bug in cppreference index page, this symbol is wrongly marked C++11.


================
Comment at: clangd/StdSymbolMap.imp:1283
+{ "std::log10", { "<cmath>", "<complex>", "<valarray>" } },
+{ "std::make_error_code", { "<system_error>", "<ios>" } }, // C++11
+{ "std::make_error_condition", { "<system_error>", "<ios>" } }, // C++11
----------------
sammccall wrote:
> we're missing the `future` variant I think?
looks like it was a bug in the generator code, fixed.


================
Comment at: clangd/StdSymbolMap.imp:1291
+{ "std::sinh", { "<cmath>", "<complex>", "<valarray>" } },
+{ "std::size_t", { "<cwchar>", "<ctime>", "<cstring>", "<cstdlib>", "<cstdio>", "<cstddef>" } },
+{ "std::sqrt", { "<cmath>", "<complex>", "<valarray>" } },
----------------
sammccall wrote:
> this is interesting: any of the options are valid. IMO the best one here is `<cstddef>`.
> But we should be picking by policy, not by looking at the header where the symbol is actually defined. We could do this on either the generator or consumer side.
this can be addressed by using a whitelist in the generator.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58345