On 30 June 2013 08:55, Tim Shen wrote: > ChangeLog entry fixed. Some comments on the latest patch (it's a long-ish email but most of my comments are very minor things regarding the _RegexMask type) ...
Defining struct _RegexMask in the typedef declaration makes it public, I think I would prefer if the struct definition was a private member of regex_traits and only the char_class_type typedef was public. Either that or just rename the type to char_class_type and do away with the typedef. I'm not sure it's necessarily required for bitmask types, but I would expect to be able to default-construct a char_class_type, is that left out intentionally? I think the _RegexMask constructor should be explicit. Its parameters should be named __base and __extended, to conform to the libstdc++ coding guidelines. It looks like _RegexMask could be a literal type, with a constexpr constructor and constexpr operators. I think we might as well make use of those features if we can. The operator functions take const parameters by value, is that a conscious decision to prefer that to either non-const parameters or pass by reference? The definition of operator== considers all bits of _M_extened to be part of the object's value representation, so: char_class_type{{},4} != char_class_type{{},0} even though those two values have the same flags set and behave identically when passed to regex_traits::isctype. It would be better to define operator== to use _M_extended&3 so only the bits we care about contribute to the value. In theory we shouldn't need a special case for the "blank" character class, I got that added to std::ctype_base via http://cplusplus.github.io/LWG/lwg-defects.html#2019, but we don't support it yet in libstdc++ (I forgot about it.) Your isctype implementation tests __f._M_extended == _RegexMask::_S_word, shouldn't that use & not ==, since __f could have both the _S_word and _S_blank bits set? Similarly for the _S_blank case. The _S_word case tests against ctype_base::alpha, but it should be alnum not alpha. I'd suggest getting rid of that test entirely though: The "w" class is represented by _RegexMask{0, _S_word}, whereas in the uncommitted patch I posted to http://gcc.gnu.org/ml/libstdc++/2010-10/msg00049.html I represented it by the equivalent of _RegexMask{ctype_base::alnum, _S_char_class_under}, which means you don't need to check the alnum case because the earlier __fctyp.is(__f._M_base, __c) test will already have handled it. That means instead of a _S_word bit you only need _S_under instead.