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

Tue Feb 19 00:11:33 PST 2019

sammccall added a comment.

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.

================
Comment at: clangd/StdSymbolMap.imp:1
+//===-- StdSymbolMap.imp - ---------------------------------------*- C++ -*-===//
+//
----------------
Even if we don't use tablegen to produce it, maybe we want to produce similar output for flexibility? e.g.
```
SYMBOL(_Exit, std::, "<cstdlib>");
```
with multiple entries (or multiple `AMBIGUOUS_SYMBOL` entries) for symbols mapped to several headers.

================
Comment at: clangd/StdSymbolMap.imp:1
+//===-- StdSymbolMap.imp - ---------------------------------------*- C++ -*-===//
+//
----------------
sammccall wrote:
> Even if we don't use tablegen to produce it, maybe we want to produce similar output for flexibility? e.g.
> ```
> SYMBOL(_Exit, std::, "<cstdlib>");
> ```
> with multiple entries (or multiple `AMBIGUOUS_SYMBOL` entries) for symbols mapped to several headers.
is there precedent for `.imp`? I think this should probably be `.inc`

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

================
Comment at: clangd/StdSymbolMap.imp:1248
+{ "std::zetta", { "<ratio>" } }, // C++11
+{ "std::abs", { "<cmath>", "<cstdlib>", "<complex>", "<valarray>" } },
+{ "std::acos", { "<cmath>", "<complex>", "<valarray>" } },
----------------
Now we come to the ambiguous cases :-)

I think it's actually quite clear what to do here:
 - the `<cmath>` and `<cstdlib>` are equivalent, but `<cmath>` is only in C++17, so we should probably prefer `<cstdlib>`
 - the `<complex>` version is written as "abs(std::complex)" in cppreference. We can tag that one with the string "complex" and prefer it if the symbol's signature contains "complex".
 - '<valarray>` is the same as `<complex>`

================
Comment at: clangd/StdSymbolMap.imp:1256
+{ "std::atanh", { "<cmath>", "<complex>" } }, // C++11
+{ "std::basic_filebuf", { "<fstream>", "<streambuf>" } },
+{ "std::consume_header", { "<codecvt>", "<locale>" } }, // C++11
----------------
I can't find a definition in <streambuf>?

================
Comment at: clangd/StdSymbolMap.imp:1257
+{ "std::basic_filebuf", { "<fstream>", "<streambuf>" } },
+{ "std::consume_header", { "<codecvt>", "<locale>" } }, // C++11
+{ "std::cos", { "<cmath>", "<complex>", "<valarray>" } },
----------------
I can't find a definition for this in <locale>?

================
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
----------------
we're missing the `future` variant I think?

================
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>" } },
----------------
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.

================
Comment at: clangd/index/CanonicalIncludes.cpp:47
+  // headers (e.g. std::move from <utility> and <algorithm>), using
+  // qualified name can not disambiguate headers. Instead we should return all
+  // headers and do the disambiguation in clangd side.
----------------
Looking at the actual data, I'm not sure this is the right strategy. (and it's not clear what "the clangd side" is :-)

The ambiguous cases seem to break down into:
 - cases where we should just pick one, e.g. cstdlib vs cmath for std::abs
 - cases where a function is overloaded for a particular type (e.g. complex), where passing more information in here (strawman: "the signature as a string") would let us get this correct. Alternatively (and easier), I believe always using the "primary" version of this function is going to be correct if not *strictly* iwyu, as the type comes from the same header. So In the short term I'd suggest just blacklisting the variants.
 - cases like std::move and... maybe one or two more? In the short term always returning `<utility>` seems good enough. In the long term, passing in the signature again would let us disambiguate here.

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58345