[PATCH] D43869: [clangd] Support include canonicalization in symbol leve.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clangd/index/CanonicalIncludes.h:47
+  /// Sets the canonical include for any symbol with \p QualifiedName.
+  /// Header mappings are ignored if \p QualifiedName matches any symbol in the
+  /// symbol mapping.
----------------
Nit: "Symbol mappings take precedence over header mappings"?


================
Comment at: clangd/index/CanonicalIncludes.h:52
+
   /// \return \p Header itself if there is no mapping for it; otherwise, return
   /// a canonical header name.
----------------
Nit: rephrase as "returns the canonical include for \p Symbol, which is declared in \p Header"?


================
Comment at: clangd/index/CanonicalIncludes.h:54
   /// a canonical header name.
-  llvm::StringRef mapHeader(llvm::StringRef Header) const;
+  /// An optional qualified symbol name can be provided to check against the
+  /// symbol mapping.
----------------
Why can't we always provide this?


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:561
+      class no_map {};
+      class ios {};
+      class ostream {};
----------------
ioeric wrote:
> hokein wrote:
> > The STL implementation of `ios` is a typedef `typedef basic_ios<char>  ios; `, I think we should make the test align with it?
> The symbol mapping doesn't make a difference between symbol types, and symbol kind test is not in the scope of this patch,  so I think this wouldn't be necessary here.
I'd be +1 on matching the real header a bit more so it's more obvious how this relates to the standard library. As long as it's not too hard!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43869