<regex>: Clean up fullmatch flag handling#6261
<regex>: Clean up fullmatch flag handling#6261muellerj2 wants to merge 2 commits intomicrosoft:mainfrom
<regex>: Clean up fullmatch flag handling#6261Conversation
StephanTLavavej
added
enhancement
| unsigned char (&_Stack_storage)[_Stack_storage_size], const bool _Full) | ||
| : _Traits(_Tr), _Begin(_Pfirst), _End(_Plast), _Sflags(_Sf), _Mflags(_Mf), _Ncap(_Nx), | ||
| _Longest((_Re->_Flags & _Fl_longest) && !(_Mf & regex_constants::match_any)) { | ||
| _Longest((_Re->_Flags & _Fl_longest) && !(_Mf & regex_constants::match_any)), _Full(_Full) { |
There was a problem hiding this comment.
Style nitpick: We conventionally avoid shadowing between parameters and data members.
| unsigned char (&_Stack_storage)[_Stack_storage_size]) | ||
| _Matcher3(_It _Pfirst, _It _Plast, const _RxTraits& _Tr, _Root_node* const _Re, const unsigned int _Nx, | ||
| const regex_constants::syntax_option_type _Sf, const regex_constants::match_flag_type _Mf, | ||
| unsigned char (&_Stack_storage)[_Stack_storage_size], const bool _Full) |
There was a problem hiding this comment.
No change requested: This is a coordinated change between member functions which is a hidden ABI risk, because you need the new constructor (which sets _Full) to be paired with calls to _Match2() (which assumes that _Full has been set). Here, we're ABI-safe because the signature change in the constructor produces a different mangled name, which is sufficient.
As you mentioned, the _Match2() rename wasn't strictly necessary because it also underwent a signature change, but I absolutely agree that the rename is desirable. For the constructor, of course we can't rename it. I don't believe there's any risk of this bool parameter disappearing during further refactoring (how would the data member get set?) so there's no need to rename the whole matcher class again.
| unsigned char (&_Stack_storage)[_Stack_storage_size], const bool _Full) | ||
| : _Traits(_Tr), _Begin(_Pfirst), _End(_Plast), _Sflags(_Sf), _Mflags(_Mf), _Ncap(_Nx), | ||
| _Longest((_Re->_Flags & _Fl_longest) && !(_Mf & regex_constants::match_any)) { | ||
| _Longest((_Re->_Flags & _Fl_longest) && !(_Mf & regex_constants::match_any)), _Full(_Full) { |
There was a problem hiding this comment.
No change requested: Because _Full doesn't change for the lifetime of the matcher, we could actually make it a template parameter and remove the data member. That would incur a class rename, and the gains would appear to be minimal.
2 participants
This PR performs some minor cleanups:
_Regex_match1()took a flag_Full, but all call sites passtrue, so we can just remove this argument. (I renamed the function even though it's not necessary, strictly speaking; there will be more changes to its signature.)_Matcher3::_Matchtook an argument_Full_match, but the value is the same for all calls on the same instance. Let's move this argument to the constructor and remove it from_Match2()(rename also not strictly necessary).constto updated function definitions._STD-qualified calls.