Page MenuHomePhabricator

[FileCheck, test] Rename checkWildcardRegexCharMatchFailure
ClosedPublic

Authored by thopre on Mar 10 2021, 6:16 AM.

Details

Summary

Proposed edit https://reviews.llvm.org/D97845#inline-922769 in D97845
suggests the checkWildcardRegexCharMatchFailure function name is
misleading because it is not clear it checks for a match failure on each
character in the string parameter. This commit renames it to an
hopefully clearer name.

Diff Detail

Event Timeline

thopre created this revision.Mar 10 2021, 6:16 AM
thopre requested review of this revision.Mar 10 2021, 6:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2021, 6:16 AM

Based on the comments in D97845, I see the semantic difference this patch addresses, but....

checkWildcardRegexCharMatchFailure uses its parameter as a sequence of
characters yet the parameter has type StringRef. This commit changes
that to SmallVector.

I usually think of a string as exactly a sequence of characters. Maybe "set of characters" better describes the goal even though the implementation is likely more efficient as a SmallVector than as a set.

The fact that AcceptedHexOnlyDigits must have a different type despite the similar name to its neighbors gives me pause. Maybe the names should be adjusted, or maybe just some comments on the variable declarations would help.

I think I'm fine with the change. It's just a bit confusing at first how a SmallVector is any better than a String here. A few tweaks like those above would probably solve that.

Based on the comments in D97845, I see the semantic difference this patch addresses, but....

checkWildcardRegexCharMatchFailure uses its parameter as a sequence of
characters yet the parameter has type StringRef. This commit changes
that to SmallVector.

I usually think of a string as exactly a sequence of characters. Maybe "set of characters" better describes the goal even though the implementation is likely more efficient as a SmallVector than as a set.

The fact that AcceptedHexOnlyDigits must have a different type despite the similar name to its neighbors gives me pause. Maybe the names should be adjusted, or maybe just some comments on the variable declarations would help.

I think I'm fine with the change. It's just a bit confusing at first how a SmallVector is any better than a String here. A few tweaks like those above would probably solve that.

The difference is to the reader. When seeing a string one usually expects something to make sense to a human as a whole (i.e. text, words). SmallVector convey that better. That said other string variables are a bit ambiguous in that they are matched as a single text but with the goal of testing matching of every single one of them in one go. Maybe I should drop this change and just find a better name for checkWildcardRegexCharMatchFailure.

Maybe I should drop this change and just find a better name for checkWildcardRegexCharMatchFailure.

Maybe checkWildcardRegexMatchPerChar?

thopre updated this revision to Diff 336049.Thu, Apr 8, 3:39 AM
thopre retitled this revision from [FileCheck] Fix checkWildcardRegexCharMatchFailure API to [FileCheck, test] Rename checkWildcardRegexCharMatchFailure.
thopre edited the summary of this revision. (Show Details)
thopre removed a subscriber: kristof.beyls.

Rename function instead

jdenny accepted this revision.Thu, Apr 8, 1:15 PM

LGTM

This revision is now accepted and ready to land.Thu, Apr 8, 1:15 PM
This revision was automatically updated to reflect the committed changes.