Patchwork Whole regex refactoring and current status

login
register
mail settings
Submitter Rainer Orth
Date Aug. 8, 2013, 12:47 p.m.
Message ID <ydd7gfwpcw5.fsf@lokon.CeBiTec.Uni-Bielefeld.DE>
Download mbox | patch
Permalink /patch/265708/
State New
Headers show

Comments

Rainer Orth - Aug. 8, 2013, 12:47 p.m.
Hi Paolo,

>>This is already documented:
>>
>>http://gcc.gnu.org/onlinedocs/libstdc++/manual/source_code_style.html#coding_style.bad_identifiers
>
> Indeed. As a simple to remember rule never use single underscore + single
> uppercase. Usually we add a p, like _Cp, etc. Plenty of examples
> everywhere. At the moment I can't do the search&replace, any such patch is
> preapproved though.

I wasn't certain about the right convention.  The following patch
allowed bootstrap to finish on i386-pc-solaris2.10 and
x86_64-unknown-linux-gnu.  I'll commit the patch once Solaris testing
has finished.

2013-08-08  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	* include/bits/regex.h: Replace _A, _B, _C, _R by _Ap, _Bp,
	_Cp, _Rp.
However, I see many 32-bit testsuite failures, both on Solaris and
Linux:

Running target unix/-m32
FAIL: 28_regex/algorithms/regex_match/basic/string_01.cc (test for excess errors)
WARNING: 28_regex/algorithms/regex_match/basic/string_01.cc compilation failed to produce executable
FAIL: 28_regex/algorithms/regex_match/basic/string_range_00_03.cc (test for excess errors)
WARNING: 28_regex/algorithms/regex_match/basic/string_range_00_03.cc compilation failed to produce executable
FAIL: 28_regex/algorithms/regex_match/basic/string_range_01_03.cc (test for excess errors)
WARNING: 28_regex/algorithms/regex_match/basic/string_range_01_03.cc compilation failed to produce executable
FAIL: 28_regex/algorithms/regex_match/basic/string_range_02_03.cc (test for excess errors)
WARNING: 28_regex/algorithms/regex_match/basic/string_range_02_03.cc compilation failed to produce executable
FAIL: 28_regex/algorithms/regex_match/extended/53622.cc (test for excess errors)
WARNING: 28_regex/algorithms/regex_match/extended/53622.cc compilation failed to produce executable
FAIL: 28_regex/algorithms/regex_match/extended/57173.cc (test for excess errors)
WARNING: 28_regex/algorithms/regex_match/extended/57173.cc compilation failed to produce executable
FAIL: 28_regex/algorithms/regex_match/extended/cstring_bracket_01.cc (test for excess errors)
WARNING: 28_regex/algorithms/regex_match/extended/cstring_bracket_01.cc compilation failed to produce executable
FAIL: 28_regex/algorithms/regex_match/extended/cstring_plus.cc (test for excess errors)
WARNING: 28_regex/algorithms/regex_match/extended/cstring_plus.cc compilation failed to produce executable
FAIL: 28_regex/algorithms/regex_match/extended/cstring_questionmark.cc (test for excess errors)
WARNING: 28_regex/algorithms/regex_match/extended/cstring_questionmark.cc compilation failed to produce executable
FAIL: 28_regex/algorithms/regex_match/extended/string_any.cc (test for excess errors)
WARNING: 28_regex/algorithms/regex_match/extended/string_any.cc compilation failed to produce executable
FAIL: 28_regex/algorithms/regex_match/extended/string_range_00_03.cc (test for excess errors)
WARNING: 28_regex/algorithms/regex_match/extended/string_range_00_03.cc compilation failed to produce executable
FAIL: 28_regex/algorithms/regex_match/extended/string_range_01_03.cc (test for excess errors)
WARNING: 28_regex/algorithms/regex_match/extended/string_range_01_03.cc compilation failed to produce executable
FAIL: 28_regex/algorithms/regex_match/extended/string_range_02_03.cc (test for excess errors)
WARNING: 28_regex/algorithms/regex_match/extended/string_range_02_03.cc compilation failed to produce executable
FAIL: 28_regex/algorithms/regex_search/basic/string_01.cc (test for excess errors)
WARNING: 28_regex/algorithms/regex_search/basic/string_01.cc compilation failed to produce executable
FAIL: 28_regex/basic_regex/ctors/basic/cstring.cc (test for excess errors)
FAIL: 28_regex/basic_regex/ctors/basic/raw_string.cc (test for excess errors)
WARNING: 28_regex/basic_regex/ctors/basic/raw_string.cc compilation failed to produce executable
FAIL: 28_regex/basic_regex/ctors/basic/string_range_01_02_03.cc (test for excess errors)
WARNING: 28_regex/basic_regex/ctors/basic/string_range_01_02_03.cc compilation failed to produce executable
FAIL: 28_regex/basic_regex/ctors/char/cstring_awk.cc (test for excess errors)
FAIL: 28_regex/basic_regex/ctors/char/cstring_egrep.cc (test for excess errors)
FAIL: 28_regex/basic_regex/ctors/char/cstring_grep.cc (test for excess errors)
FAIL: 28_regex/basic_regex/ctors/extended/cstring.cc (test for excess errors)
FAIL: 28_regex/basic_regex/ctors/extended/string_range_01_02_03.cc (test for excess errors)
WARNING: 28_regex/basic_regex/ctors/extended/string_range_01_02_03.cc compilation failed to produce executable

E.g. the first one is

FAIL: 28_regex/algorithms/regex_match/basic/string_01.cc (test for excess errors)
Excess errors:
/vol/gcc/src/hg/trunk/local/libstdc++-v3/testsuite/28_regex/algorithms/regex_match/basic/string_01.cc:34:47: error: call of overloaded 'basic_regex(const char [8], const flag_type&)' is ambiguous

The other failures are similar.

	Rainer
Paolo Carlini - Aug. 8, 2013, 12:53 p.m.
Hi,

>I wasn't certain about the right convention.  The following patch
>allowed bootstrap to finish on i386-pc-solaris2.10 and
>x86_64-unknown-linux-gnu.  I'll commit the patch once Solaris testing
>has finished.

Thanks a lot.

>E.g. the first one is
>
>FAIL: 28_regex/algorithms/regex_match/basic/string_01.cc (test for
>excess errors)
>Excess errors:
>/vol/gcc/src/hg/trunk/local/libstdc++-v3/testsuite/28_regex/algorithms/regex_match/basic/string_01.cc:34:47:
>error: call of overloaded 'basic_regex(const char [8], const
>flag_type&)' is ambiguous
>
>The other failures are similar.

Tim, please address this ASAP, otherwise we have to revert the whole thing.

Paolo
Kyrylo Tkachov - Aug. 8, 2013, 1:19 p.m.
> Hi Paolo,
> 
> >>This is already documented:
> >>
> >>http://gcc.gnu.org/onlinedocs/libstdc++/manual/source_code_style.html#c
> oding_style.bad_identifiers
> >
> > Indeed. As a simple to remember rule never use single underscore +
> single
> > uppercase. Usually we add a p, like _Cp, etc. Plenty of examples
> > everywhere. At the moment I can't do the search&replace, any such patch
> is
> > preapproved though.
> 
> I wasn't certain about the right convention.  The following patch
> allowed bootstrap to finish on i386-pc-solaris2.10 and
> x86_64-unknown-linux-gnu.  I'll commit the patch once Solaris testing
> has finished.

I can confirm that this patch fixes the arm-none-eabi build as well.

Thanks,
Kyrill
Tim Shen - Aug. 8, 2013, 2:04 p.m.
On Thu, Aug 8, 2013 at 8:53 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Tim, please address this ASAP, otherwise we have to revert the whole thing.

I'm trying to reproduce the compilation failures.
Tim Shen - Aug. 8, 2013, 2:22 p.m.
On Thu, Aug 8, 2013 at 10:04 PM, Tim Shen <timshen91@gmail.com> wrote:
> On Thu, Aug 8, 2013 at 8:53 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> Tim, please address this ASAP, otherwise we have to revert the whole thing.
>
> I'm trying to reproduce the compilation failures.

There's a typedef in regex_constants.h:

`typedef unsigned int syntax_option_type;`

Which is a little bit naive. It possibly conflicts with size_t under
i386 when overloading. I'm trying to change it to a bitset. Or is
there any better solution?
Paolo Carlini - Aug. 8, 2013, 3:14 p.m.
Hi,

>There's a typedef in regex_constants.h:
>
>`typedef unsigned int syntax_option_type;`
>
>Which is a little bit naive. It possibly conflicts with size_t under
>i386 when overloading. I'm trying to change it to a bitset. Or is
>there any better solution?

In my humble opinion involving the whole std::bitset container for a syntax option is way overkill. Do you really have to do overloading between size_t and that type? Or maybe you can use a type *smaller* than unsigned int.

Paolo
Tim Shen - Aug. 8, 2013, 3:20 p.m.
On Thu, Aug 8, 2013 at 11:14 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> In my humble opinion involving the whole std::bitset container for a syntax option is way overkill. Do you really have to do overloading between size_t and that type? Or maybe you can use a type *smaller* than unsigned int.

The n3376 standard specified the following constructor:

...
explicit basic_regex(const charT* p, flag_type f = regex_constants::ECMAScript);
basic_regex(const charT* p, size_t len, flag_type f =
regex_constants::ECMAScript);
...

So flag_type shall not be the same as size_t. I don't know if when I
switch flag_type from unsigned int to, say, unsigned short, conflicts
will appear in 16bit archtectures.
Andreas Schwab - Aug. 8, 2013, 3:39 p.m.
Paolo Carlini <paolo.carlini@oracle.com> writes:

> In my humble opinion involving the whole std::bitset container for a
> syntax option is way overkill.

It's already used for match_flag_type anyway.

Andreas.
Paolo Carlini - Aug. 8, 2013, 4:11 p.m.
Hi,

>So flag_type shall not be the same as size_t. I don't know if when I
>switch flag_type from unsigned int to, say, unsigned short, conflicts
>will appear in 16bit archtectures.

Well 16-bit archs aren't really supported these days, as long as the c++ runtime is concerned. Thus if unsigned short works let's go with it and move on. In any case, we don't have to make a super final decision, but breaking 32 bit is bad.

In general, I would recommend being careful with adding templated containers, remember that eventually we want to have a lot of code in *.cc files and instantiations will be a big big annoyance.

Paolo
Paolo Carlini - Aug. 8, 2013, 4:15 p.m.
Hi,

>> In my humble opinion involving the whole std::bitset container for a
>> syntax option is way overkill.
>
>It's already used for match_flag_type anyway.

Indeed, I noticed it a few days ago, and seemed overkill ;) Really, we already have implementation experience about these flags, eg in iostream, and I dont think we want std::bitset everywhere.

Paolo

Paolo
Tim Shen - Aug. 8, 2013, 4:51 p.m.
On Fri, Aug 9, 2013 at 12:15 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Indeed, I noticed it a few days ago, and seemed overkill ;) Really, we already have implementation experience about these flags, eg in iostream, and I dont think we want std::bitset everywhere.

So here's the change. It's under testing now, but could took several
hours. If someone has a faster machine, please tell the result :)

As a simple test, `g++ -m32 test.cc` didn't fail.
Paolo Carlini - Aug. 8, 2013, 5:24 p.m.
On 08/08/2013 06:51 PM, Tim Shen wrote:
> On Fri, Aug 9, 2013 at 12:15 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> Indeed, I noticed it a few days ago, and seemed overkill ;) Really, we already have implementation experience about these flags, eg in iostream, and I dont think we want std::bitset everywhere.
> So here's the change. It's under testing now, but could took several
> hours. If someone has a faster machine, please tell the result :)
>
> As a simple test, `g++ -m32 test.cc` didn't fail.
Thanks!

Paolo.
Tim Shen - Aug. 9, 2013, 5:08 a.m.
On Fri, Aug 9, 2013 at 12:51 AM, Tim Shen <timshen91@gmail.com> wrote:
> So here's the change. It's under testing now, but could took several
> hours. If someone has a faster machine, please tell the result :)

Unfortuantely using `unsigned int => unsigned short` cannot work.
Instead, I create a enum and do some operator overloadings.

This patch passed bootstrap and all tests under the configuration under x86_64:

--with-arch=corei7 --with-cpu=corei7 --prefix=/usr/local
--enable-clocale=gnu --with-system-zlib --enable-shared
--with-demangler-in-ld --with-fpmath=sse --enable-languages=c++
Paolo Carlini - Aug. 9, 2013, 6:59 a.m.
Hi,

On 08/09/2013 07:08 AM, Tim Shen wrote:
> On Fri, Aug 9, 2013 at 12:51 AM, Tim Shen <timshen91@gmail.com> wrote:
>> So here's the change. It's under testing now, but could took several
>> hours. If someone has a faster machine, please tell the result :)
> Unfortuantely using `unsigned int => unsigned short` cannot work.
> Instead, I create a enum and do some operator overloadings.
>
> This patch passed bootstrap and all tests under the configuration under x86_64:
>
> --with-arch=corei7 --with-cpu=corei7 --prefix=/usr/local
> --enable-clocale=gnu --with-system-zlib --enable-shared
> --with-demangler-in-ld --with-fpmath=sse --enable-languages=c++
Yes, if, as it should, it works on -m32 too, let's go with this. By the 
way, you didn't say how exactly you are testing?!? Because just make 
check doesn't test -m32. I use, something like:

make -k check-target-libstdc++-v3 
RUNTESTFLAGS="--target_board='unix/\{-m32,-m64\}'"

for sure there are other ways to obtain the same result.

Paolo.
Tim Shen - Aug. 9, 2013, 7:57 a.m.
On Fri, Aug 9, 2013 at 2:59 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Yes, if, as it should, it works on -m32 too, let's go with this. By the way,
> you didn't say how exactly you are testing?!? Because just make check
> doesn't test -m32. I use, something like:
>
> make -k check-target-libstdc++-v3
> RUNTESTFLAGS="--target_board='unix/\{-m32,-m64\}'"
>
> for sure there are other ways to obtain the same result.

Committed.

I entered `gcc-build/x86_64-unknown-linux-gnu/32/libstdc++-v3` then
run `make check`. I see -m32 in test log.
Rainer Orth - Aug. 9, 2013, 11:37 a.m.
Tim Shen <timshen91@gmail.com> writes:

> On Fri, Aug 9, 2013 at 2:59 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> Yes, if, as it should, it works on -m32 too, let's go with this. By the way,
>> you didn't say how exactly you are testing?!? Because just make check
>> doesn't test -m32. I use, something like:
>>
>> make -k check-target-libstdc++-v3
>> RUNTESTFLAGS="--target_board='unix/\{-m32,-m64\}'"
>>
>> for sure there are other ways to obtain the same result.
>
> Committed.
>
> I entered `gcc-build/x86_64-unknown-linux-gnu/32/libstdc++-v3` then
> run `make check`. I see -m32 in test log.

As expected, i386-pc-solaris2.10 testsuite results are back to normal
now.

Thanks.
        Rainer

Patch

diff --git a/libstdc++-v3/include/bits/regex.h b/libstdc++-v3/include/bits/regex.h
--- a/libstdc++-v3/include/bits/regex.h
+++ b/libstdc++-v3/include/bits/regex.h
@@ -995,18 +995,18 @@  namespace std _GLIBCXX_VISIBILITY(defaul
                                  const basic_regex<_CharT, _TraitsT>&,
                                  regex_constants::match_flag_type);
 
-      template<typename _B, typename _A, typename _C, typename _R>
+      template<typename _Bp, typename _Ap, typename _Cp, typename _Rp>
         friend bool
-        regex_match(_B, _B,
-                    match_results<_B, _A>&,
-                    const basic_regex<_C, _R>&,
+        regex_match(_Bp, _Bp,
+                    match_results<_Bp, _Ap>&,
+                    const basic_regex<_Cp, _Rp>&,
                     regex_constants::match_flag_type);
 
-      template<typename _B, typename _A, typename _C, typename _R>
+      template<typename _Bp, typename _Ap, typename _Cp, typename _Rp>
         friend bool
-        regex_search(_B, _B,
-                     match_results<_B, _A>&,
-                     const basic_regex<_C, _R>&,
+        regex_search(_Bp, _Bp,
+                     match_results<_Bp, _Ap>&,
+                     const basic_regex<_Cp, _Rp>&,
                      regex_constants::match_flag_type);
 
       flag_type     _M_flags;
@@ -2111,16 +2111,16 @@  namespace std _GLIBCXX_VISIBILITY(defaul
       template<typename, typename, typename, typename>
         friend class __detail::_BFSExecutor;
 
-      template<typename _B, typename _A, typename _Ch_type, typename _Rx_traits>
+      template<typename _Bp, typename _Ap, typename _Ch_type, typename _Rx_traits>
         friend bool
-        regex_match(_B, _B, match_results<_B, _A>&,
+        regex_match(_Bp, _Bp, match_results<_Bp, _Ap>&,
                     const basic_regex<_Ch_type,
                     _Rx_traits>&,
                     regex_constants::match_flag_type);
 
-      template<typename _B, typename _A, typename _Ch_type, typename _Rx_traits>
+      template<typename _Bp, typename _Ap, typename _Ch_type, typename _Rx_traits>
         friend bool
-        regex_search(_B, _B, match_results<_B, _A>&,
+        regex_search(_Bp, _Bp, match_results<_Bp, _Ap>&,
                      const basic_regex<_Ch_type,
                      _Rx_traits>&,
                      regex_constants::match_flag_type);