[PATCH] D24027: [LLVM/Support] - Make match() method of llvm::Regex to be const

[David Blaikie via llvm-commits]

(or remove the side-channel error handling, and have the APIs return errors
explicitly instead - the side channel's likely to be accidentally unchecked
by many callers, which is probably a mistake)

On Tue, Aug 30, 2016 at 8:43 AM David Blaikie <dblaikie at gmail.com> wrote:

> Sounds like the error handling is part of the identity of the object - I'm
> not sure it'd be a good idea to make that mutable & make the object
> logically const when it's actually changing that error field. (it seems
> it'd be easy to introduce misuse there - where one user does something to
> the Regex, then passes a const ref somewhere, then examines the error field
> and finds it changed - let alone multithreading)
>
> Perhaps the right fix is for you to use MutableArrayRef instead?
>
> On Tue, Aug 30, 2016 at 6:52 AM George Rimar <grimar at accesssoftek.com>
> wrote:
>
>> grimar created this revision.
>> grimar added reviewers: rafael, davide, dblaikie, ruiu.
>> grimar added subscribers: llvm-commits, grimar.
>>
>> This can help to D23829.
>> I have a std::vector of Regex and use ArrayRefs to pass it.
>> The fact of match() is not const forces me to use const_cast for
>> removing it. Also there is no way to call match() on
>> Regex const reference, at the same time the only thing match()
>> can change is error field and method name and job it do does not sounds
>> like something not-const.
>>
>>
>> https://reviews.llvm.org/D24027
>>
>> Files:
>>   include/llvm/Support/Regex.h
>>   lib/Support/Regex.cpp
>>
>> Index: lib/Support/Regex.cpp
>> ===================================================================
>> --- lib/Support/Regex.cpp
>> +++ lib/Support/Regex.cpp
>> @@ -56,7 +56,7 @@
>>    return preg->re_nsub;
>>  }
>>
>> -bool Regex::match(StringRef String, SmallVectorImpl<StringRef> *Matches){
>> +bool Regex::match(StringRef String, SmallVectorImpl<StringRef> *Matches)
>> const {
>>    unsigned nmatch = Matches ? preg->re_nsub+1 : 0;
>>
>>    // pmatch needs to have at least one element.
>> Index: include/llvm/Support/Regex.h
>> ===================================================================
>> --- include/llvm/Support/Regex.h
>> +++ include/llvm/Support/Regex.h
>> @@ -74,7 +74,8 @@
>>      /// the first group is always the entire pattern.
>>      ///
>>      /// This returns true on a successful match.
>> -    bool match(StringRef String, SmallVectorImpl<StringRef> *Matches =
>> nullptr);
>> +    bool match(StringRef String,
>> +               SmallVectorImpl<StringRef> *Matches = nullptr) const;
>>
>>      /// sub - Return the result of replacing the first match of the
>> regex in
>>      /// \p String with the \p Repl string. Backreferences like "\0" in
>> the
>> @@ -98,7 +99,7 @@
>>
>>    private:
>>      struct llvm_regex *preg;
>> -    int error;
>> +    mutable int error;
>>    };
>>  }
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160830/c4a0a971/attachment.html>