On 26/03/19 15:28 +0000, Jonathan Wakely wrote:
The static assertions added for PR libstdc++/48101 were at class scope
and so were evaluated too eagerly, when it might not be possible to
determine whether the function objects are invocable with the key types.
The problematic cases are where the key type is not known to be
convertible to the argument type(s) of the function object until later,
after a type has been completed. Specifically, if the key type is a
pointer to a derived class and the function object's argument type is a
pointer to a base class, then the derived-to-base conversion is only
valid once the derived type is complete.
By moving the static assertions to the destructor they will only be
evaluated when the destructor is instantiated, at which point whether
the key type can be passed to the function object should be knowable.
The ideal place to do the checks would be only when the function objects
are actually invoked, but that would mean adding the checks in numerous
places, so the destructor is used instead.
The tests need to be adjusted because the "required from here" line is
now the location of the destructor, not the point of instantiation in
the test file. For the map and multimap tests which check two
specializations, the dg-error matching the assertion text matches both
cases. Also check the diagnostic output for the template arguments, to
ensure both specializations trigger the assertion.
PR libstdc++/85965
* include/bits/hashtable.h (_Hashtable): Move static assertions to
destructor so they are not evaluated until the _Key type is complete.
* include/bits/stl_tree.h (_Rb_tree): Likewise.
* testsuite/23_containers/set/85965.cc: New test.
* testsuite/23_containers/unordered_set/85965.cc: New test.
* testsuite/23_containers/map/48101_neg.cc: Replace "here" errors
with regexp matching the corresponding _Rb_tree specialization.
* testsuite/23_containers/multimap/48101_neg.cc: Likewise.
* testsuite/23_containers/multiset/48101_neg.cc: Remove "here" error.
* testsuite/23_containers/set/48101_neg.cc: Likewise.
* testsuite/23_containers/unordered_map/48101_neg.cc: Likewise.
* testsuite/23_containers/unordered_multimap/48101_neg.cc: Likewise.
* testsuite/23_containers/unordered_multiset/48101_neg.cc: Likewise.
* testsuite/23_containers/unordered_set/48101_neg.cc: Likewise.
Tested powerpc64le-linux, committed to trunk.
Backport for gcc-8-branch to follow.
That patch (r269949 on trunk) fixed the original PR 85965 example, but
there are other uses of maps and sets where checking in the destructor
is still too strict.
This is another attempt to reduce how often the assertions are
evaluated, so that code which doesn't try to use the function objects
doesn't need them to be invocable.
For _Rb_tree we access the _M_key_compare object directly, so can't put
the assertions in an accessor function for it. However, every invocation
of _M_key_compare is accompanied by a use of _S_key, so the assertions
can be put in there. For _Hashtable there are member functions that are
consistently used to obtain a hash code or test for equality, so the
assertions can go in those members.
PR libstdc++/85965
* include/bits/hashtable.h (_Hashtable::~_Hashtable()): Remove static
assertions from the destructor.
* include/bits/hashtable_policy.h (_Hash_code_base::_M_hash_code):
Move static_assert for hash function to here.
(_Hash_table_base::_M_equals): Move static_assert for equality
predicate to here.
* include/bits/stl_tree.h (_Rb_tree::_S_value(_Const_Link_type)):
Remove.
(_Rb_tree::_S_key(_Const_Link_type)): Move assertions here. Access
the value directly instead of calling _S_value.
(_Rb_tree::_S_value(_Const_Base_ptr)): Remove.
(_Rb_tree::_S_key(_Const_Base_ptr)): Do downcast and forward to
_S_key(_Const_Link_type).
* testsuite/23_containers/set/85965.cc: Check construction,
destruction, assignment and size() do not trigger the assertions.
* testsuite/23_containers/unordered_set/85965.cc: Likewise.
* testsuite/23_containers/map/48101_neg.cc: Call find and adjust
expected errors.
* testsuite/23_containers/multimap/48101_neg.cc: Likewise.
* testsuite/23_containers/multiset/48101_neg.cc: Likewise.
* testsuite/23_containers/set/48101_neg.cc: Likewise.
* testsuite/23_containers/unordered_map/48101_neg.cc: Likewise.
* testsuite/23_containers/unordered_multimap/48101_neg.cc: Likewise.
* testsuite/23_containers/unordered_multiset/48101_neg.cc: Likewise.
* testsuite/23_containers/unordered_set/48101_neg.cc: Likewise.
This makes it possible for function objects to be unusable in any
member functions of the containers that doesn't actually perform
lookups, e.g. construction, destruction, assignment, empty(), size().
Unfortunately the error messages aren't actually very nice with this
version. Because the static_assert is delayed until the last moment,
the actual invalid invocation of the function object is still
compiled, and still gives a screenful of template instantiation
errors. You get the static assertion error *as well*, and it appears
to come last, where it shouldn't get drowned in noise, but it's not as
user-friendly as I hoped it would be.
But it does fix the regressions where reasonable code was rejected,
and the static assertion message as well as template barf seems better
than just template barf.
I tried SFINAEing away the functions containing the static assertions,
so the invalid invocations would never happen, but that doesn't seem
to produce better results.
Anybody got a better idea, or should I commit this?
commit daec5e7122c0a201f1e7579b7c4e01c31cc5cec7
Author: Jonathan Wakely <jwak...@redhat.com>
Date: Wed May 15 15:10:53 2019 +0100
PR libstdc++/85965 move is_invocable assertions again
This is another attempt to reduce how often the assertions are
evaluated, so that code which doesn't try to use the function objects
doesn't need them to be invocable.
For _Rb_tree we access the _M_key_compare object directly, so can't put
the assertions in an accessor function for it. However, every invocation
of _M_key_compare is accompanied by a use of _S_key, so the assertions
can be put in there. For _Hashtable there are member functions that are
consistently used to obtain a hash code or test for equality, so the
assertions can go in those members.
PR libstdc++/85965
* include/bits/hashtable.h (_Hashtable::~_Hashtable()): Remove static
assertions from the destructor.
* include/bits/hashtable_policy.h (_Hash_code_base::_M_hash_code):
Move static_assert for hash function to here.
(_Hash_table_base::_M_equals): Move static_assert for equality
predicate to here.
* include/bits/stl_tree.h (_Rb_tree::_S_value(_Const_Link_type)):
Remove.
(_Rb_tree::_S_key(_Const_Link_type)): Move assertions here. Access
the value directly instead of calling _S_value.
(_Rb_tree::_S_value(_Const_Base_ptr)): Remove.
(_Rb_tree::_S_key(_Const_Base_ptr)): Do downcast and forward to
_S_key(_Const_Link_type).
* testsuite/23_containers/set/85965.cc: Check construction,
destruction, assignment and size() do not trigger the assertions.
* testsuite/23_containers/unordered_set/85965.cc: Likewise.
* testsuite/23_containers/map/48101_neg.cc: Call find and adjust
expected errors.
* testsuite/23_containers/multimap/48101_neg.cc: Likewise.
* testsuite/23_containers/multiset/48101_neg.cc: Likewise.
* testsuite/23_containers/set/48101_neg.cc: Likewise.
* testsuite/23_containers/unordered_map/48101_neg.cc: Likewise.
* testsuite/23_containers/unordered_multimap/48101_neg.cc: Likewise.
* testsuite/23_containers/unordered_multiset/48101_neg.cc: Likewise.
* testsuite/23_containers/unordered_set/48101_neg.cc: Likewise.
diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h
index 2e8aeb7e4d4..ab24b5bb537 100644
--- a/libstdc++-v3/include/bits/hashtable.h
+++ b/libstdc++-v3/include/bits/hashtable.h
@@ -1351,12 +1351,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
{
clear();
_M_deallocate_buckets();
-
- static_assert(__is_invocable<const _H1&, const _Key&>{},
- "hash function must be invocable with an argument of key type");
- static_assert(__is_invocable<const _Equal&, const _Key&, const _Key&>{},
- "key equality predicate must be invocable with two arguments of "
- "key type");
}
template<typename _Key, typename _Value,
diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h
index c7f466cd686..9505fb8d261 100644
--- a/libstdc++-v3/include/bits/hashtable_policy.h
+++ b/libstdc++-v3/include/bits/hashtable_policy.h
@@ -1298,7 +1298,11 @@ namespace __detail
__hash_code
_M_hash_code(const _Key& __k) const
- { return _M_h1()(__k); }
+ {
+ static_assert(__is_invocable<const _H1&, const _Key&>{},
+ "hash function must be invocable with an argument of key type");
+ return _M_h1()(__k);
+ }
std::size_t
_M_bucket_index(const _Key&, __hash_code __c, std::size_t __n) const
@@ -1386,7 +1390,11 @@ namespace __detail
__hash_code
_M_hash_code(const _Key& __k) const
- { return _M_h1()(__k); }
+ {
+ static_assert(__is_invocable<const _H1&, const _Key&>{},
+ "hash function must be invocable with an argument of key type");
+ return _M_h1()(__k);
+ }
std::size_t
_M_bucket_index(const _Key&, __hash_code __c,
@@ -1832,6 +1840,9 @@ namespace __detail
bool
_M_equals(const _Key& __k, __hash_code __c, __node_type* __n) const
{
+ static_assert(__is_invocable<const _Equal&, const _Key&, const _Key&>{},
+ "key equality predicate must be invocable with two arguments of "
+ "key type");
return _EqualHelper::_S_equals(_M_eq(), this->_M_extract(),
__k, __c, __n);
}
diff --git a/libstdc++-v3/include/bits/stl_tree.h b/libstdc++-v3/include/bits/stl_tree.h
index 00e4a0cccbf..940155d8a32 100644
--- a/libstdc++-v3/include/bits/stl_tree.h
+++ b/libstdc++-v3/include/bits/stl_tree.h
@@ -759,13 +759,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_M_end() const _GLIBCXX_NOEXCEPT
{ return &this->_M_impl._M_header; }
- static const_reference
- _S_value(_Const_Link_type __x)
- { return *__x->_M_valptr(); }
-
static const _Key&
_S_key(_Const_Link_type __x)
- { return _KeyOfValue()(_S_value(__x)); }
+ {
+#if __cplusplus >= 201103L
+ // If we're asking for the key we're presumably using the comparison
+ // object, and so this is a good place to sanity check it.
+ static_assert(__is_invocable<_Compare&, const _Key&, const _Key&>{},
+ "comparison object must be invocable "
+ "with two arguments of key type");
+# if __cplusplus >= 201703L
+ // _GLIBCXX_RESOLVE_LIB_DEFECTS
+ // 2542. Missing const requirements for associative containers
+ if constexpr (__is_invocable<_Compare&, const _Key&, const _Key&>{})
+ static_assert(
+ is_invocable_v<const _Compare&, const _Key&, const _Key&>,
+ "comparison object must be invocable as const");
+# endif // C++17
+#endif // C++11
+
+ return _KeyOfValue()(*__x->_M_valptr());
+ }
static _Link_type
_S_left(_Base_ptr __x) _GLIBCXX_NOEXCEPT
@@ -783,13 +797,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_S_right(_Const_Base_ptr __x) _GLIBCXX_NOEXCEPT
{ return static_cast<_Const_Link_type>(__x->_M_right); }
- static const_reference
- _S_value(_Const_Base_ptr __x)
- { return *static_cast<_Const_Link_type>(__x)->_M_valptr(); }
-
static const _Key&
_S_key(_Const_Base_ptr __x)
- { return _KeyOfValue()(_S_value(__x)); }
+ { return _S_key(static_cast<_Const_Link_type>(__x)); }
static _Base_ptr
_S_minimum(_Base_ptr __x) _GLIBCXX_NOEXCEPT
@@ -974,21 +984,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
#endif
~_Rb_tree() _GLIBCXX_NOEXCEPT
- {
- _M_erase(_M_begin());
-
-#if __cplusplus >= 201103L
- static_assert(__is_invocable<_Compare&, const _Key&, const _Key&>{},
- "comparison object must be invocable "
- "with two arguments of key type");
-# if __cplusplus >= 201703L
- // _GLIBCXX_RESOLVE_LIB_DEFECTS
- // 2542. Missing const requirements for associative containers
- static_assert(is_invocable_v<const _Compare&, const _Key&, const _Key&>,
- "comparison object must be invocable as const");
-# endif // C++17
-#endif // C++11
- }
+ { _M_erase(_M_begin()); }
_Rb_tree&
operator=(const _Rb_tree& __x);
diff --git a/libstdc++-v3/testsuite/23_containers/map/48101_neg.cc b/libstdc++-v3/testsuite/23_containers/map/48101_neg.cc
index 355222058ff..8a7429c85a8 100644
--- a/libstdc++-v3/testsuite/23_containers/map/48101_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/map/48101_neg.cc
@@ -24,9 +24,13 @@ void
test01()
{
std::map<int, int, std::less<int*>> c;
+ c.find(1); // { dg-error "here" }
std::map<int, int, std::allocator<int>> c2;
+ c2.find(2); // { dg-error "here" }
}
// { dg-error "_Compare = std::less<int.>" "" { target *-*-* } 0 }
// { dg-error "_Compare = std::allocator<int>" "" { target *-*-* } 0 }
// { dg-error "comparison object must be invocable" "" { target *-*-* } 0 }
+// { dg-prune-output "no match for call" }
+// { dg-prune-output "invalid conversion" }
diff --git a/libstdc++-v3/testsuite/23_containers/multimap/48101_neg.cc b/libstdc++-v3/testsuite/23_containers/multimap/48101_neg.cc
index 5f53ccee168..7bd56cc9c73 100644
--- a/libstdc++-v3/testsuite/23_containers/multimap/48101_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/multimap/48101_neg.cc
@@ -24,9 +24,13 @@ void
test01()
{
std::multimap<int, int, std::less<int*>> c;
+ c.find(1); // { dg-error "here" }
std::multimap<int, int, std::allocator<int>> c2;
+ c2.find(2); // { dg-error "here" }
}
// { dg-error "_Compare = std::less<int.>" "" { target *-*-* } 0 }
// { dg-error "_Compare = std::allocator<int>" "" { target *-*-* } 0 }
// { dg-error "comparison object must be invocable" "" { target *-*-* } 0 }
+// { dg-prune-output "no match for call" }
+// { dg-prune-output "invalid conversion" }
diff --git a/libstdc++-v3/testsuite/23_containers/multiset/48101_neg.cc b/libstdc++-v3/testsuite/23_containers/multiset/48101_neg.cc
index 71b90e80951..f13aa098976 100644
--- a/libstdc++-v3/testsuite/23_containers/multiset/48101_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/multiset/48101_neg.cc
@@ -24,9 +24,12 @@ test01()
{
std::multiset<const int> c; // { dg-error "here" }
std::multiset<int, std::less<long*>> c2;
+ c2.find(2); // { dg-error "here" }
}
// { dg-error "non-const, non-volatile value_type" "" { target *-*-* } 0 }
// { dg-error "comparison object must be invocable" "" { target *-*-* } 0 }
// { dg-prune-output "std::allocator<.* has no member named " }
// { dg-prune-output "must have the same value_type as its allocator" }
+// { dg-prune-output "no match for call" }
+// { dg-prune-output "invalid conversion" }
diff --git a/libstdc++-v3/testsuite/23_containers/set/48101_neg.cc b/libstdc++-v3/testsuite/23_containers/set/48101_neg.cc
index e58c0627eb8..4ede0421d90 100644
--- a/libstdc++-v3/testsuite/23_containers/set/48101_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/set/48101_neg.cc
@@ -24,9 +24,12 @@ test01()
{
std::set<const int> c; // { dg-error "here" }
std::set<int, std::less<long*>> c2;
+ c2.find(2); // { dg-error "here" }
}
// { dg-error "non-const, non-volatile value_type" "" { target *-*-* } 0 }
// { dg-error "comparison object must be invocable" "" { target *-*-* } 0 }
// { dg-prune-output "std::allocator<.* has no member named " }
// { dg-prune-output "must have the same value_type as its allocator" }
+// { dg-prune-output "no match for call" }
+// { dg-prune-output "invalid conversion" }
diff --git a/libstdc++-v3/testsuite/23_containers/set/85965.cc b/libstdc++-v3/testsuite/23_containers/set/85965.cc
index 54d501f6c4f..7d8f2167519 100644
--- a/libstdc++-v3/testsuite/23_containers/set/85965.cc
+++ b/libstdc++-v3/testsuite/23_containers/set/85965.cc
@@ -27,3 +27,12 @@ struct Foo
// PR libstdc++/85965
std::set<Derived*, std::less<Base*>> s;
};
+
+std::size_t
+test01(std::set<Derived*, std::less<Base*>> s)
+{
+ // these operations should not require the comparison object
+ auto copy = s;
+ copy = s;
+ return s.size();
+}
diff --git a/libstdc++-v3/testsuite/23_containers/unordered_map/48101_neg.cc b/libstdc++-v3/testsuite/23_containers/unordered_map/48101_neg.cc
index 6c3092554f3..8d823dfa476 100644
--- a/libstdc++-v3/testsuite/23_containers/unordered_map/48101_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/unordered_map/48101_neg.cc
@@ -24,8 +24,10 @@ test01()
{
using namespace std;
unordered_map<int, int, equal_to<int>, hash<int>> c2;
+ c2.find(2); // { dg-error "here" }
}
// { dg-error "hash function must be invocable" "" { target *-*-* } 0 }
// { dg-error "key equality predicate must be invocable" "" { target *-*-* } 0 }
// { dg-prune-output "use of deleted function" }
+// { dg-prune-output "no match for call" }
diff --git a/libstdc++-v3/testsuite/23_containers/unordered_multimap/48101_neg.cc b/libstdc++-v3/testsuite/23_containers/unordered_multimap/48101_neg.cc
index f5de313a8f1..a81615b3607 100644
--- a/libstdc++-v3/testsuite/23_containers/unordered_multimap/48101_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/unordered_multimap/48101_neg.cc
@@ -24,8 +24,10 @@ test01()
{
using namespace std;
unordered_multimap<int, int, equal_to<int>, hash<int>> c2;
+ c2.find(2); // { dg-error "here" }
}
// { dg-error "hash function must be invocable" "" { target *-*-* } 0 }
// { dg-error "key equality predicate must be invocable" "" { target *-*-* } 0 }
// { dg-prune-output "use of deleted function" }
+// { dg-prune-output "no match for call" }
diff --git a/libstdc++-v3/testsuite/23_containers/unordered_multiset/48101_neg.cc b/libstdc++-v3/testsuite/23_containers/unordered_multiset/48101_neg.cc
index d4e479aaf97..03ddb898d6c 100644
--- a/libstdc++-v3/testsuite/23_containers/unordered_multiset/48101_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/unordered_multiset/48101_neg.cc
@@ -25,6 +25,7 @@ test01()
using namespace std;
unordered_multiset<const int, hash<int>> c; // { dg-error "here" }
unordered_multiset<int, equal_to<int>, hash<int>> c2;
+ c2.find(2); // { dg-error "here" }
}
// { dg-error "non-const, non-volatile value_type" "" { target *-*-* } 0 }
@@ -32,3 +33,4 @@ test01()
// { dg-error "key equality predicate must be invocable" "" { target *-*-* } 0 }
// { dg-prune-output "use of deleted function" }
// { dg-prune-output "must have the same value_type as its allocator" }
+// { dg-prune-output "no match for call" }
diff --git a/libstdc++-v3/testsuite/23_containers/unordered_set/48101_neg.cc b/libstdc++-v3/testsuite/23_containers/unordered_set/48101_neg.cc
index c7e27c53690..e79d3769248 100644
--- a/libstdc++-v3/testsuite/23_containers/unordered_set/48101_neg.cc
+++ b/libstdc++-v3/testsuite/23_containers/unordered_set/48101_neg.cc
@@ -25,6 +25,7 @@ test01()
using namespace std;
unordered_set<const int, hash<int>> c; // { dg-error "here" }
unordered_set<int, equal_to<int>, hash<int>> c2;
+ c2.find(2); // { dg-error "here" }
}
// { dg-error "non-const, non-volatile value_type" "" { target *-*-* } 0 }
@@ -32,3 +33,4 @@ test01()
// { dg-error "key equality predicate must be invocable" "" { target *-*-* } 0 }
// { dg-prune-output "use of deleted function" }
// { dg-prune-output "must have the same value_type as its allocator" }
+// { dg-prune-output "no match for call" }
diff --git a/libstdc++-v3/testsuite/23_containers/unordered_set/85965.cc b/libstdc++-v3/testsuite/23_containers/unordered_set/85965.cc
index 8b90b369901..8c48fa2a978 100644
--- a/libstdc++-v3/testsuite/23_containers/unordered_set/85965.cc
+++ b/libstdc++-v3/testsuite/23_containers/unordered_set/85965.cc
@@ -27,3 +27,12 @@ struct Foo
// PR libstdc++/85965
std::unordered_set<Derived*, std::equal_to<Base*>, std::hash<Base*>> u;
};
+
+std::size_t
+test01(std::unordered_set<Derived*, std::equal_to<Base*>, std::hash<Base*>> s)
+{
+ // these operations should not require the comparison object
+ auto copy = s;
+ copy = s;
+ return s.size();
+}
