| Message ID | 20190515153716.GX2599@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Remove redundant accessors in hash tables | expand |
On 5/15/19 5:37 PM, Jonathan Wakely wrote: > François, > I noticed that _Hash_code_base and _Hashtable_base have a number of > member functions which are overloaded for const and non-const: > > const _Equal& > _M_eq() const { return _EqualEBO::_S_cget(*this); } > > _Equal& > _M_eq() { return _EqualEBO::_S_get(*this); } > > The non-const ones seem to be unnecessary. They're used in the _M_swap > member functions, but all other uses could (and probably should) call > the const overload to get a const reference to the function object. > > If we make the _M_swap members use the EBO accessors directly then we > can get rid of the non-const accessors. That makes overload resolution > simpler for the compiler (as there's only one function to choose from) > and should result in slightly smaller code when inlining is not > enabled. > > Do you see any problem with this patch? > > I think it is more a Pavlov behavior, always providing const and non-const no matter what. No problem to simplify this.
On 16/05/19 07:47 +0200, François Dumont wrote: >On 5/15/19 5:37 PM, Jonathan Wakely wrote: >>François, >>I noticed that _Hash_code_base and _Hashtable_base have a number of >>member functions which are overloaded for const and non-const: >> >> const _Equal& >> _M_eq() const { return _EqualEBO::_S_cget(*this); } >> >> _Equal& >> _M_eq() { return _EqualEBO::_S_get(*this); } >> >>The non-const ones seem to be unnecessary. They're used in the _M_swap >>member functions, but all other uses could (and probably should) call >>the const overload to get a const reference to the function object. >> >>If we make the _M_swap members use the EBO accessors directly then we >>can get rid of the non-const accessors. That makes overload resolution >>simpler for the compiler (as there's only one function to choose from) >>and should result in slightly smaller code when inlining is not >>enabled. >> >>Do you see any problem with this patch? >> >> >I think it is more a Pavlov behavior, always providing const and >non-const no matter what. > >No problem to simplify this. OK, tested powerpc64le-linux, committed to trunk.
On 16/05/19 11:05 +0100, Jonathan Wakely wrote: >On 16/05/19 07:47 +0200, François Dumont wrote: >>On 5/15/19 5:37 PM, Jonathan Wakely wrote: >>>François, >>>I noticed that _Hash_code_base and _Hashtable_base have a number of >>>member functions which are overloaded for const and non-const: >>> >>> const _Equal& >>> _M_eq() const { return _EqualEBO::_S_cget(*this); } >>> >>> _Equal& >>> _M_eq() { return _EqualEBO::_S_get(*this); } >>> >>>The non-const ones seem to be unnecessary. They're used in the _M_swap >>>member functions, but all other uses could (and probably should) call >>>the const overload to get a const reference to the function object. >>> >>>If we make the _M_swap members use the EBO accessors directly then we >>>can get rid of the non-const accessors. That makes overload resolution >>>simpler for the compiler (as there's only one function to choose from) >>>and should result in slightly smaller code when inlining is not >>>enabled. >>> >>>Do you see any problem with this patch? >>> >>> >>I think it is more a Pavlov behavior, always providing const and >>non-const no matter what. >> >>No problem to simplify this. > >OK, tested powerpc64le-linux, committed to trunk. I don't see a need for the _Hashtable_ebo_helper member functions to be static. They return a member of *this, but are static so *this has to be passed as a parameter. We could just make them non-static. It seems to have always been that way since the first version of the patch that added the helpers: https://gcc.gnu.org/ml/libstdc++/2011-12/msg00139.html This patch passes all tests. I plan to commit this to trunk too.
On 16/05/19 13:30 +0100, Jonathan Wakely wrote: >On 16/05/19 11:05 +0100, Jonathan Wakely wrote: >>On 16/05/19 07:47 +0200, François Dumont wrote: >>>On 5/15/19 5:37 PM, Jonathan Wakely wrote: >>>>François, >>>>I noticed that _Hash_code_base and _Hashtable_base have a number of >>>>member functions which are overloaded for const and non-const: >>>> >>>> const _Equal& >>>> _M_eq() const { return _EqualEBO::_S_cget(*this); } >>>> >>>> _Equal& >>>> _M_eq() { return _EqualEBO::_S_get(*this); } >>>> >>>>The non-const ones seem to be unnecessary. They're used in the _M_swap >>>>member functions, but all other uses could (and probably should) call >>>>the const overload to get a const reference to the function object. >>>> >>>>If we make the _M_swap members use the EBO accessors directly then we >>>>can get rid of the non-const accessors. That makes overload resolution >>>>simpler for the compiler (as there's only one function to choose from) >>>>and should result in slightly smaller code when inlining is not >>>>enabled. >>>> >>>>Do you see any problem with this patch? >>>> >>>> >>>I think it is more a Pavlov behavior, always providing const and >>>non-const no matter what. >>> >>>No problem to simplify this. >> >>OK, tested powerpc64le-linux, committed to trunk. > >I don't see a need for the _Hashtable_ebo_helper member functions to >be static. They return a member of *this, but are static so *this has >to be passed as a parameter. We could just make them non-static. > >It seems to have always been that way since the first version of the >patch that added the helpers: >https://gcc.gnu.org/ml/libstdc++/2011-12/msg00139.html > >This patch passes all tests. I plan to commit this to trunk too. And another one (which I've actually had sitting in a local branch for a few years!)
diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h index 9505fb8d261..5ff90c952ba 100644 --- a/libstdc++-v3/include/bits/hashtable_policy.h +++ b/libstdc++-v3/include/bits/hashtable_policy.h @@ -1229,21 +1229,16 @@ namespace __detail void _M_swap(_Hash_code_base& __x) { - std::swap(_M_extract(), __x._M_extract()); - std::swap(_M_ranged_hash(), __x._M_ranged_hash()); + std::swap(__ebo_extract_key::_S_get(*this), + __ebo_extract_key::_S_get(__x)); + std::swap(__ebo_hash::_S_get(*this), __ebo_hash::_S_get(__x)); } const _ExtractKey& _M_extract() const { return __ebo_extract_key::_S_cget(*this); } - _ExtractKey& - _M_extract() { return __ebo_extract_key::_S_get(*this); } - const _Hash& _M_ranged_hash() const { return __ebo_hash::_S_cget(*this); } - - _Hash& - _M_ranged_hash() { return __ebo_hash::_S_get(*this); } }; // No specialization for ranged hash function while caching hash codes. @@ -1326,28 +1321,20 @@ namespace __detail void _M_swap(_Hash_code_base& __x) { - std::swap(_M_extract(), __x._M_extract()); - std::swap(_M_h1(), __x._M_h1()); - std::swap(_M_h2(), __x._M_h2()); + std::swap(__ebo_extract_key::_S_get(*this), + __ebo_extract_key::_S_get(__x)); + std::swap(__ebo_h1::_S_get(*this), __ebo_h1::_S_get(__x)); + std::swap(__ebo_h2::_S_get(*this), __ebo_h2::_S_get(__x)); } const _ExtractKey& _M_extract() const { return __ebo_extract_key::_S_cget(*this); } - _ExtractKey& - _M_extract() { return __ebo_extract_key::_S_get(*this); } - const _H1& _M_h1() const { return __ebo_h1::_S_cget(*this); } - _H1& - _M_h1() { return __ebo_h1::_S_get(*this); } - const _H2& _M_h2() const { return __ebo_h2::_S_cget(*this); } - - _H2& - _M_h2() { return __ebo_h2::_S_get(*this); } }; /// Specialization: hash function and range-hashing function, @@ -1418,28 +1405,20 @@ namespace __detail void _M_swap(_Hash_code_base& __x) { - std::swap(_M_extract(), __x._M_extract()); - std::swap(_M_h1(), __x._M_h1()); - std::swap(_M_h2(), __x._M_h2()); + std::swap(__ebo_extract_key::_S_get(*this), + __ebo_extract_key::_S_get(__x)); + std::swap(__ebo_h1::_S_get(*this), __ebo_h1::_S_get(__x)); + std::swap(__ebo_h2::_S_get(*this), __ebo_h2::_S_get(__x)); } const _ExtractKey& _M_extract() const { return __ebo_extract_key::_S_cget(*this); } - _ExtractKey& - _M_extract() { return __ebo_extract_key::_S_get(*this); } - const _H1& _M_h1() const { return __ebo_h1::_S_cget(*this); } - _H1& - _M_h1() { return __ebo_h1::_S_get(*this); } - const _H2& _M_h2() const { return __ebo_h2::_S_cget(*this); } - - _H2& - _M_h2() { return __ebo_h2::_S_get(*this); } }; /** @@ -1851,14 +1830,11 @@ namespace __detail _M_swap(_Hashtable_base& __x) { __hash_code_base::_M_swap(__x); - std::swap(_M_eq(), __x._M_eq()); + std::swap(_EqualEBO::_S_get(*this), _EqualEBO::_S_get(__x)); } const _Equal& _M_eq() const { return _EqualEBO::_S_cget(*this); } - - _Equal& - _M_eq() { return _EqualEBO::_S_get(*this); } }; /**
François, I noticed that _Hash_code_base and _Hashtable_base have a number of member functions which are overloaded for const and non-const: const _Equal& _M_eq() const { return _EqualEBO::_S_cget(*this); } _Equal& _M_eq() { return _EqualEBO::_S_get(*this); } The non-const ones seem to be unnecessary. They're used in the _M_swap member functions, but all other uses could (and probably should) call the const overload to get a const reference to the function object. If we make the _M_swap members use the EBO accessors directly then we can get rid of the non-const accessors. That makes overload resolution simpler for the compiler (as there's only one function to choose from) and should result in slightly smaller code when inlining is not enabled. Do you see any problem with this patch?