[PATCH] D87468: [Support] Add GlobPattern::isTrivialMatchAll()

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/include/llvm/Support/GlobPattern.h:35
+  // Returns true for glob pattern "*".
+  bool isTrivialMatchAll() const { return Prefix && Prefix->empty(); }
+
----------------
andrewng wrote:
> MaskRay wrote:
> > This is not correct (`*foo`). `Suffix` may be non-empty.
> > 
> > 
> `Prefix` has priority over `Suffix` in the `GlobPattern` creation code, which is why this code works. The current code doesn't support both a `Prefix` and a `Suffix`.
I think this needs a comment, saying that this member can be used "to avoid cost in preparing/acquiring the input"
when it is expensive. Otherwise it is probably not clear why it is exist/needed.

Not strong suggestions/thoughts:
1) Perhaps, worth to add `assert(!Suffix`);` to emphasize we don't expect it?

```
if (Prefix && Prefix->empty()) {
  assert(!Suffix);
  return true;
}

return false;
```

2) Instead of having `isTrivialMatchAll`, we could add a member (+ `getPatternString` getter) to store the
original pattern string and just check it is equal to the `*` on the callers side. It can be a bit more generic API then.


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

https://reviews.llvm.org/D87468