[PATCH] D87468: [Support] Add GlobPattern::isTrivialMatchAll()
Andrew Ng via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 11 07:52:53 PDT 2020
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
More information about the llvm-commits
mailing list