Page MenuHomePhabricator

[Support] Add GlobPattern::isTrivialMatchAll()
ClosedPublic

Authored by andrewng on Thu, Sep 10, 10:15 AM.

Details

Summary

GlobPattern::isTrivialMatchAll() returns true for the GlobPattern "*"
which will match all inputs.

This can be used to avoid performing expensive preparation of the input
for match() when the result of the match will always be true.

Diff Detail

Event Timeline

andrewng created this revision.Thu, Sep 10, 10:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Sep 10, 10:15 AM
andrewng requested review of this revision.Thu, Sep 10, 10:15 AM

This is a prerequisite of D87469.

GlobPattern::matchOne has a fast path for '*'. Isn't it fast enough?

llvm/include/llvm/Support/GlobPattern.h
35

This is not correct (*foo). Suffix may be non-empty.

andrewng marked an inline comment as done.Thu, Sep 10, 10:52 AM

GlobPattern::matchOne has a fast path for '*'. Isn't it fast enough?

As mentioned in the summary, this is to avoid cost in preparing/acquiring the input, i.e. not the speed of the match but overhead related to the input.

llvm/include/llvm/Support/GlobPattern.h
35

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.

GlobPattern::matchOne has a fast path for '*'. Isn't it fast enough?

As mentioned in the summary, this is to avoid cost in preparing/acquiring the input, i.e. not the speed of the match but overhead related to the input.

Can you demonstrate how the old code has overhead related to the input?

andrewng marked an inline comment as done.Thu, Sep 10, 11:17 AM

Can you demonstrate how the old code has overhead related to the input?

This new function enables the user of the GlobPattern to avoid overhead, see D87469 for an example.

grimar added inline comments.Fri, Sep 11, 2:34 AM
llvm/include/llvm/Support/GlobPattern.h
35

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;
  1. 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.

andrewng updated this revision to Diff 291217.Fri, Sep 11, 7:48 AM
andrewng edited the summary of this revision. (Show Details)

Updated to address review comments and suggestions.

andrewng marked an inline comment as done.Fri, Sep 11, 7:52 AM
andrewng added inline comments.
llvm/include/llvm/Support/GlobPattern.h
35

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.

MaskRay accepted this revision.Fri, Sep 11, 10:20 AM

I've checked the dependent patch. This looks reasonable. Please wait for @grimar as well.

This revision is now accepted and ready to land.Fri, Sep 11, 10:20 AM
grimar accepted this revision.Mon, Sep 14, 1:28 AM

LGTM

MaskRay added inline comments.Mon, Sep 14, 5:26 PM
llvm/unittests/Support/GlobPatternTest.cpp
141

Most patterns are expected to return false.

The negative patterns can be organized as a const char * array to improve readability

141

You don't need EXPECT_TRUE((bool)Pat2); because it is implied by the following Pat2->

andrewng updated this revision to Diff 291904.Tue, Sep 15, 7:06 AM
andrewng marked an inline comment as done.

Update for review comments.

andrewng marked 2 inline comments as done.Tue, Sep 15, 7:15 AM
andrewng added inline comments.
llvm/unittests/Support/GlobPatternTest.cpp
141

Although it is implied by the -> operator, it feels better to keep the check separate and this is the pattern followed by the other tests in this file.

MaskRay added inline comments.Tue, Sep 15, 9:51 AM
llvm/unittests/Support/GlobPatternTest.cpp
143

You can use a range-based for here.

andrewng updated this revision to Diff 291972.Tue, Sep 15, 10:55 AM
andrewng marked an inline comment as done.

Update for review suggestion.

andrewng marked an inline comment as done.Tue, Sep 15, 10:55 AM
andrewng added inline comments.
llvm/unittests/Support/GlobPatternTest.cpp
143

Thanks for spotting that one!

This revision was automatically updated to reflect the committed changes.
andrewng marked an inline comment as done.