[PATCH] D98343: [FileCheck] Fix checkWildcardRegexCharMatchFailure API

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added a comment.

In D98343#2618052 <https://reviews.llvm.org/D98343#2618052>, @jdenny wrote:

> Based on the comments in D97845 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98343