Re: [patch] regex_traits implementation

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

On 2 July 2013 20:10, Tim Shen wrote:
> On Wed, Jul 3, 2013 at 1:11 AM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>> I think the _RegexMask constructor should be explicit.
>
> I intentionally make _RegexMask act like an unsigned number. Since
> lookup_collatename is required to return a value compares equal to 0,

Oh, I hadn't noticed that requirement. I am inclined to consider that
a defect in the standard, it assumes an integral type, which bit-mask
types are not required to be.  The standard should probably say it
compares equal to a value-initialized object instead, but given what
it says your choice to allow implicit conversions makes sense.

> I don't know if adding operater==(ctype_base __n) a good idea. It's a
> little bit elegant to make this implicit conver

Adding that operator wouldn't work for 0==x, only for x==0, so you'd
want a non-member, but let's keep the implicit conversion anyway.


>> Its parameters should be named __base and __extended, to conform to the libstdc++
>> coding guidelines.
>
> Fixed.

Sorry for not being clear, the class data members should be _M_xxx but
the function parameters should be __xxx, they should not use the same
naming convention. That makes it clear when referring to a
(non-public) member rather than a local variable or function
parameter.   The libstdc++ style guidelines are at
http://gcc.gnu.org/onlinedocs/libstdc++/manual/source_code_style.html


>> 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?
>
> It's intended.
>
> First, since sizeof(_RegexMask) is relatively small, a call-by-value
> call may save more time than pointer-based reference;

All the functions are inline, so it's unlikely to make much difference
in either case, both the reference binding and copying are likely to
be optimized away entirely except when using no optimization at all.

> Second, making it `const` enables more potential compiler optimizations.

As I said in my previous reply, I don't think this is true.

>> 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.)
>
> We don't have std::isblank yet; I add a comment for this.

Great, thanks.  We should try to add isblank for GCC 4.9, so might be
able to revisit your regex_traits to use it before the release.  If we
remove it, it might be better to remove the 1<<1 bit and keep 1<<0, so
it would make more sense to swap the values of _S_blank and _S_under.

I've just noticed that the try-catch block in transform_primary should
use the macros __try and __catch, which will allow the code to be used
with -fno-exceptions