[PATCH] D143274: [clangd] Remove the direct use of StdSymbolMapping.inc usage.

Mon Feb 6 05:18:06 PST 2023

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