hokein marked an inline comment as done.
hokein added a comment.
In D143274#4106449 <https://reviews.llvm.org/D143274#4106449>, @kadircet wrote:
> thanks a lot. since this is the last (and only) upstream user of the raw
> mappings. can you also move them into `clang/lib/Tooling/Inclusions/Stdlib/`
> as part of this patch?
As discussed, I will address this in a followup patch.
================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.cpp:711
+ return "<utility>";
+ // There are multiple headers for size_t, pick one.
+ if (QName == "std::size_t")
----------------
kadircet wrote:
> i think the comment is misleading. as if we had some alternatives in the
> tooling::stdlib, it would already pick one for us. the issue is we don't have
> it in the mapping at all.
yeah, this comment was moved from the old version. I removed it as it is
covered by the above FIXME.
================
Comment at: clang-tools-extra/clangd/index/CanonicalIncludes.h:43
+ /// Returns the overridden include for a qualified symbol with, or "".
+ llvm::StringRef mapSymbol(llvm::StringRef /*std::*/ Scope,
+ llvm::StringRef /*vector*/ Name,
----------------
kadircet wrote:
> these parameter comments are a little bit unconventional, maybe just mention
> that `Scope` should have trailing colons (e.g. std::) and name shouldn't have
> any (e.g. vector) ? also mention that `it should be empty ("") for global
> namespace`?
Removed the parameter comments, and refined the doc comment of the function.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143274/new/
https://reviews.llvm.org/D143274
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
