[PATCH] PR libstdc++/85965 delay static assertions until types are complete

[Jonathan Wakely]

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?


-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.txt
Type: text/x-patch
Size: 15098 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20190515/8c92db2b/attachment.bin>