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