Re: [patch] regex_traits implementation

[Jonathan Wakely <jwakely dot gcc at gmail dot com>]

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.