[PATCH] D135226: [clangd] Optimize Dex::generateProximityURIs().

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:358
+// Returns        ^
+const char *findPathInURI(const char *S) {
+  // Skip over scheme.
----------------
adamcz wrote:
> Is there a reason why you're doing this with manual manipulation of C-strings instead of StringRefs? I suspect passing StringRef here was not the problem, turning it into an std::string, from another std::string inside ParsedURI was.
> 
> Something like:
> S = S.drop_while([](char c) { return c == ':'; });
> S.consume_front(":");
> if (S.consume_front("//")) {
>   S = S.drop_while([](char c) { return c == '/'; });
>   S.consume_front("/");
> }
> 
> return S;
> 
> would be a bit more readable and less likely to have an off-by-one somewhere, imho.
I started with StringRef, and found it too awkward. I've changed it back, but I think it makes the main algorithm harder to understand (it's OK for findPathInURI, but mixing styles is more confusing for me).

StringRef is good at modelling range content or two pointers, but this algorithm is fundamentally about three pointers and uses (Begin, End) and (Path, End) equally. So you end up with too many degrees of freedom in the code and have to paper over with assertions.
And StringRef's API is mostly suited for manipulating content, not dealing with overlapping refs, so you end up with a mix of index+pointer+stringref that personally I find harder to understand.

I'm probably an outlier here, so converted it to use StringRef anyway


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135226/new/

https://reviews.llvm.org/D135226