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