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

[Andrew Ng via Phabricator via llvm-commits]

andrewng marked an inline comment as done.
andrewng added inline comments.


================
Comment at: llvm/include/llvm/Support/GlobPattern.h:35
+  // Returns true for glob pattern "*".
+  bool isTrivialMatchAll() const { return Prefix && Prefix->empty(); }
+
----------------
grimar wrote:
> 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.
Added comment and adopted suggestion of the assertion on `Suffix`, thanks. Generally, I don't think it would be worthwhile storing the original pattern just for this use case.


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

https://reviews.llvm.org/D87468