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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
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.
----------------
sammccall wrote:
> Why can't we always provide this?
I think it would be better to keep supporting the header-only mapping use case.


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:561
+      class no_map {};
+      class ios {};
+      class ostream {};
----------------
sammccall wrote:
> 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!
I'd prefer to keep the test case minimum so that it's clear that we are only matching on names.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43869