[PATCH] D156046: [Support] Rewrite GlobPattern

[Ellis Hoag via Phabricator via llvm-commits]

ellis 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);
----------------
Will this fail for the pattern `abc\\` (ending with an escaped backslash)?


================
Comment at: llvm/lib/Support/GlobPattern.cpp:75
+    if (S[I] == '[') {
+      size_t J = S.find(']', I + 1);
+      if (J == StringRef::npos)
----------------
>From line 77:
```
 // ']' is allowed as the first character of a character class. '[]' is
 // invalid. So, just skip the first character.
 size_t End = S.find(']', 2);
```

I would expect this to break the `BracketFrontOfCharacterClass` test


================
Comment at: llvm/lib/Support/GlobPattern.cpp:77
+      if (J == StringRef::npos)
+        return make_error<StringError>("invalid glob pattern: " + S,
+                                       errc::invalid_argument);
----------------
Can we add something like `expected ']'` to this error message?


================
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;
----------------
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?


================
Comment at: llvm/lib/Support/GlobPattern.cpp:139
+    // position to probe the next starting position after the last '*'.
+    P = Star;
+    S = ++SavedS;
----------------
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)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156046