[PATCH] D156046: [Support] Rewrite GlobPattern

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Support/GlobPattern.cpp:67-69
+  if (S.endswith("\\"))
+    return make_error<StringError>("invalid glob pattern, stray '\\'",
+                                   errc::invalid_argument);
----------------
ellis wrote:
> Will this fail for the pattern `abc\\` (ending with an escaped backslash)?
Thanks for catching this. We need to handle this when scanning the pattern.


================
Comment at: llvm/lib/Support/GlobPattern.cpp:77
+      if (J == StringRef::npos)
+        return make_error<StringError>("invalid glob pattern: " + S,
+                                       errc::invalid_argument);
----------------
ellis wrote:
> Can we add something like `expected ']'` to this error message?
I'd like to do that, but 6 lld/test/ELF tests rely on the particular error message. The change should probably be made in the future...

I think we should drop `+ S` from GlobPattern.cpp diagnostics. The clients can add `+ S` as needed.


================
Comment at: llvm/lib/Support/GlobPattern.cpp:105-106
+bool GlobPattern::matchOne(StringRef Str) const {
+  const char *P = Pat.data(), *Star = nullptr, *S = Str.data(), *SavedS = S;
+  const char *const PEnd = P + Pat.size(), *const End = S + Str.size();
+  size_t B = 0, SavedB = 0;
----------------
ellis wrote:
> Since you are storing `P`, `PEnd` and `S`, `End` anyway, why not store these as `StringRef`s so there are fewer variables to keep track of?
Below we are going to increment various variables, mainly P, S, and B.
If we save `P, PEnd` as a `StringRef`, the increment and backtracking will be more cumbersome...


================
Comment at: llvm/lib/Support/GlobPattern.cpp:139
+    // position to probe the next starting position after the last '*'.
+    P = Star;
+    S = ++SavedS;
----------------
ellis wrote:
> To me it's confusing that `Star` would hold the index after the `*`. Could we make this `P = Star + 1` and make line 114 `Star = P++` (or separate it into two lines)?
Thank's for the question. I renamed `Star` to `SegmentBegin`, hopefully it is clearer.

Using `P = SegmentBegin;` will be slightly more efficient than `P = LastStar + 1;` as backtracking has one less variable to increment...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156046