diff mbox

Avoid #ifdef _GLIBCXX_DEBUG in regex_compiler.h

Message ID 20150907145425.GL2631@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely Sept. 7, 2015, 2:54 p.m. UTC 
We could go further and remove the ABI difference in regex_compiler
when using _GLIBCXX_DEBUG, as in the attached patch.

The trick is that for the char specialization we have padding bytes
between _M_is_non_matching (a bool) and _M_cache (a std::bitset), so
we can reuse one of those padding bytes for the _M_is_ready flag.

For the non-char specializations the _Dummy struct takes up one byte,
so we could reuse that for the _M_is_ready flag.

To be safe we should probably check that alignof(std::bitset<8> > 2)
so we know there really is a padding byte that can be re-used, and
place the _M_is_ready flag afterwards if there is no padding.

Comments

Jonathan Wakely Sept. 7, 2015, 3:22 p.m. UTC | #1 
On 07/09/15 15:54 +0100, Jonathan Wakely wrote:
>@@ -457,10 +457,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	return 1ul << (sizeof(_CharT) * __CHAR_BIT__ * int(_UseCache::value));
>       }
>
>-      struct _Dummy { };
>-      typedef typename std::conditional<_UseCache::value,
>-					std::bitset<_S_cache_size()>,
>-					_Dummy>::type _CacheT;
>+      struct _Flags
>+      {
>+	bool _M_is_non_matching;
>+	bool _M_is_ready;
>+      };
>+
>+      struct _Empty { };
>+
>+      struct _ExtraMembers
>+      : _Flags,
>+	conditional<_UseCache::value, bitset<_S_cache_size()>, _Empty>::type
>+      {
>+	explicit
>+	_ExtraMembers(bool __is_non_matching)
>+	: _Flags{__is_non_matching, false}
>+	{ }
>+      };
>+

And we could get rid of the _Empty type, because std::bitset<0> is an
empty type anyway, so if we made _S_cache_size()==0 when _UseCache is
false then in the current code we could just unconditionally use:

  using _CacheT = std::bitset<_S_cache_size()>;

and for the suggested change to use a padding byte for the _M_is_ready
flag we could use:

      struct _ExtraMembers
      : _Flags, bitset<_S_cache_size()>
      {
        explicit
        _ExtraMembers(bool __is_non_matching)
        : _Flags{__is_non_matching, false}
        { }
      };
Tim Shen Sept. 15, 2015, 4:54 a.m. UTC | #2 
On Mon, Sep 7, 2015 at 8:22 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
> And we could get rid of the _Empty type, because std::bitset<0> is an
> empty type anyway, so if we made _S_cache_size()==0 when _UseCache is
> false then in the current code we could just unconditionally use:
>
>  using _CacheT = std::bitset<_S_cache_size()>;
>
> and for the suggested change to use a padding byte for the _M_is_ready
> flag we could use:
>
>      struct _ExtraMembers
>      : _Flags, bitset<_S_cache_size()>
>      {
>        explicit
>        _ExtraMembers(bool __is_non_matching)
>        : _Flags{__is_non_matching, false}
>        { }
>      };

I'm not fully awaring the context, but I planned to elimiate the cache
size dispatch based on char types by always using size 256 (to be
exact, the size of all possible unsigned char values) to cache the
smallest 256 results, regardless of the actual char type. It also
covers other char types and reduced the code complexity.

As for #ifdef _GLIBCXX_DEBUG, I think it's fine to delete them, since
they seem not catching any useful bugs.
Jonathan Wakely Sept. 15, 2015, 9:36 a.m. UTC | #3 
On 14/09/15 21:54 -0700, Tim Shen wrote:
>As for #ifdef _GLIBCXX_DEBUG, I think it's fine to delete them, since
>they seem not catching any useful bugs.

Sounds good to me.
diff mbox

Patch

diff --git a/libstdc++-v3/include/bits/regex_compiler.h b/libstdc++-v3/include/bits/regex_compiler.h
index 07a9ed3..9616d4e 100644
--- a/libstdc++-v3/include/bits/regex_compiler.h
+++ b/libstdc++-v3/include/bits/regex_compiler.h
@@ -369,13 +369,13 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _BracketMatcher(bool __is_non_matching,
 		      const _TraitsT& __traits)
       : _M_class_set(0), _M_translator(__traits), _M_traits(__traits),
-      _M_is_non_matching(__is_non_matching)
+      _M_extra_members(__is_non_matching)
       { }
 
       bool
       operator()(_CharT __ch) const
       {
-	_GLIBCXX_DEBUG_ASSERT(_M_is_ready);
+	_GLIBCXX_DEBUG_ASSERT(_M_extra._M_is_ready);
 	return _M_apply(__ch, _UseCache());
       }
 
@@ -383,7 +383,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _M_add_char(_CharT __c)
       {
 	_M_char_set.push_back(_M_translator._M_translate(__c));
-	_GLIBCXX_DEBUG_ONLY(_M_is_ready = false);
+	_GLIBCXX_DEBUG_ONLY(_M_extra._M_is_ready = false);
       }
 
       _StringT
@@ -394,7 +394,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	if (__st.empty())
 	  __throw_regex_error(regex_constants::error_collate);
 	_M_char_set.push_back(_M_translator._M_translate(__st[0]));
-	_GLIBCXX_DEBUG_ONLY(_M_is_ready = false);
+	_GLIBCXX_DEBUG_ONLY(_M_extra._M_is_ready = false);
 	return __st;
       }
 
@@ -408,7 +408,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	__st = _M_traits.transform_primary(__st.data(),
 					   __st.data() + __st.size());
 	_M_equiv_set.push_back(__st);
-	_GLIBCXX_DEBUG_ONLY(_M_is_ready = false);
+	_GLIBCXX_DEBUG_ONLY(_M_extra._M_is_ready = false);
       }
 
       // __neg should be true for \D, \S and \W only.
@@ -424,7 +424,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  _M_class_set |= __mask;
 	else
 	  _M_neg_class_set.push_back(__mask);
-	_GLIBCXX_DEBUG_ONLY(_M_is_ready = false);
+	_GLIBCXX_DEBUG_ONLY(_M_extra._M_is_ready = false);
       }
 
       void
@@ -434,7 +434,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  __throw_regex_error(regex_constants::error_range);
 	_M_range_set.push_back(make_pair(_M_translator._M_transform(__l),
 					 _M_translator._M_transform(__r)));
-	_GLIBCXX_DEBUG_ONLY(_M_is_ready = false);
+	_GLIBCXX_DEBUG_ONLY(_M_extra._M_is_ready = false);
       }
 
       void
@@ -444,7 +444,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	auto __end = std::unique(_M_char_set.begin(), _M_char_set.end());
 	_M_char_set.erase(__end, _M_char_set.end());
 	_M_make_cache(_UseCache());
-	_GLIBCXX_DEBUG_ONLY(_M_is_ready = true);
+	_GLIBCXX_DEBUG_ONLY(_M_extra._M_is_ready = true);
       }
 
     private:
@@ -457,10 +457,24 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	return 1ul << (sizeof(_CharT) * __CHAR_BIT__ * int(_UseCache::value));
       }
 
-      struct _Dummy { };
-      typedef typename std::conditional<_UseCache::value,
-					std::bitset<_S_cache_size()>,
-					_Dummy>::type _CacheT;
+      struct _Flags
+      {
+	bool _M_is_non_matching;
+	bool _M_is_ready;
+      };
+
+      struct _Empty { };
+
+      struct _ExtraMembers
+      : _Flags,
+	conditional<_UseCache::value, bitset<_S_cache_size()>, _Empty>::type
+      {
+	explicit
+	_ExtraMembers(bool __is_non_matching)
+	: _Flags{__is_non_matching, false}
+	{ }
+      };
+
       typedef typename std::make_unsigned<_CharT>::type _UnsignedCharT;
 
       bool
@@ -468,13 +482,14 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       bool
       _M_apply(_CharT __ch, true_type) const
-      { return _M_cache[static_cast<_UnsignedCharT>(__ch)]; }
+      { return _M_extra._M_cache[static_cast<_UnsignedCharT>(__ch)]; }
 
       void
       _M_make_cache(true_type)
       {
-	for (unsigned __i = 0; __i < _M_cache.size(); __i++)
-	  _M_cache[__i] = _M_apply(static_cast<_CharT>(__i), false_type());
+	auto& __cache = _M_extra._M_cache;
+	for (unsigned __i = 0; __i < __cache.size(); __i++)
+	  __cache[__i] = _M_apply(static_cast<_CharT>(__i), false_type());
       }
 
       void
@@ -489,11 +504,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _CharClassT                               _M_class_set;
       _TransT                                   _M_translator;
       const _TraitsT&                           _M_traits;
-      bool                                      _M_is_non_matching;
-      _CacheT					_M_cache;
-#ifdef _GLIBCXX_DEBUG
-      bool                                      _M_is_ready = false;
-#endif
+      _ExtraMembers				_M_extra;
     };
 
  //@} regex-detail