[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clangd/index/Index.h:93
+    return Cmp < 0;
+  return std::tie(L.Start, L.End) < std::tie(R.Start, R.End);
 }
----------------
sammccall wrote:
> tie(strcmp(L.FileURI, R.FileURI), ...) < tie(0, ...)?
tie doesn't support this usage, it expects lvalue arguments.


================
Comment at: clangd/index/Index.h:306
+    llvm::StringRef S(P);
+    CB(S);
+    P = S.data();
----------------
sammccall wrote:
> ```assert (!S.data()[S.size()] && "Visited StringRef must be null-terminated")
Does this assert actually work? The `StringRef` is constructed from the `char *` (which using `strlen`). This assert will not get triggered I think, `S.data()[S.size()]` is always `\0`.


================
Comment at: clangd/index/Merge.cpp:118
   bool MergeIncludes =
-      L.Definition.FileURI.empty() == R.Definition.FileURI.empty();
+      !std::strlen(L.Definition.FileURI) == !std::strlen(R.Definition.FileURI);
   Symbol S = PreferR ? R : L;        // The target symbol we're merging into.
----------------
sammccall wrote:
> Since this reads awkwardly anyway...
> ```bool(*L.Definition.FileURI) == bool(*R.definition.FileURI)```
> is shorter and faster
this is neat!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53427