On Fri, 18 Apr 2025 at 23:08, Jason Merrill <ja...@redhat.com> wrote:
>
> limit_bad_template_recursion currently avoids immediate instantiation of
> templates from uses in an already ill-formed instantiation, but we still can
> get unnecessary recursive instantiation in pending_templates if the
> instantiation was queued before the error.
>
> Currently this regresses several libstdc++ tests which seem to rely on a
> static_assert in a function called from another that is separately ill-formed.
> For instance, in the 48101_neg.cc tests, we first get an error in find(), then
> later instantiate _S_key() (called from find) and get the static_assert error
> from there.
I think the current placement of the is_invocable static_assert tested
for by map/48101_neg.cc works because of the current G++ behaviour,
but if changing the G++ behaviour makes sense (and it seems reasonable
to me) then we could put the static_assert somewhere else.
Maybe instead of calling _M_impl._M_key_compare directly in find, we
could add a new member that does that call, and put the static_assert
in there.
If I add this to stl_tree.h and move the static_asserts from _S_key
into this new function:
template<typename _Key1, typename _Key2>
bool
_M_key_compare(const _Key1& __k1, const _Key2& __k2) const
{
return _M_impl._M_key_compare(__k1, __k2);
}
So that it looks like this:
template<typename _Key1, typename _Key2>
bool
_M_key_compare(const _Key1& __k1, const _Key2& __k2) const
{
#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 _M_impl._M_key_compare(__k1, __k2);
}
and then update the existing calls to _M_impl._M_key_compare(x,y) to
call _M_key_compare(x,y) instead, the errors are much shorter and
clearer. The first error is the static_assert (and that would
presumably still be true after your change).
So I think we can adjust the library and its tests.
>
> Thoughts? Is this a desirable change, or is the fact that the use precedes
> the
> error reason to go ahead with the instantiation?
>
> > FAIL: 23_containers/map/48101_neg.cc -std=gnu++17 (test for errors, line )
> > FAIL: 23_containers/multimap/48101_neg.cc -std=gnu++17 (test for errors,
> > line )
> > FAIL: 23_containers/multiset/48101_neg.cc -std=gnu++17 (test for errors,
> > line )
> > FAIL: 23_containers/set/48101_neg.cc -std=gnu++17 (test for errors, line )
> > FAIL: 30_threads/packaged_task/cons/dangling_ref.cc -std=gnu++17 (test
> > for errors, line )
> > FAIL: 30_threads/packaged_task/cons/lwg4154_neg.cc -std=gnu++17 (test for
> > errors, line )
>
>
> gcc/cp/ChangeLog:
>
> * cp-tree.h (struct tinst_level): Add had_errors bit.
> * pt.cc (push_tinst_level_loc): Clear it.
> (pop_tinst_level): Set it.
> (reopen_tinst_level): Check it.
> (instantiate_pending_templates): Call limit_bad_template_recursion.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/template/recurse5.C: New test.
> ---
> gcc/cp/cp-tree.h | 10 ++++++++--
> gcc/cp/pt.cc | 10 ++++++++--
> gcc/testsuite/g++.dg/template/recurse5.C | 17 +++++++++++++++++
> 3 files changed, 33 insertions(+), 4 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/template/recurse5.C
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 7798efba3db..856202c65dd 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -6755,8 +6755,14 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
> /* The location where the template is instantiated. */
> location_t locus;
>
> - /* errorcount + sorrycount when we pushed this level. */
> - unsigned short errors;
> + /* errorcount + sorrycount when we pushed this level. If the value
> + overflows, it will always seem like we currently have more errors, so we
> + will limit template recursion even from non-erroneous templates. In a
> TU
> + with over 32k errors, that's fine. */
> + unsigned short errors : 15;
> +
> + /* set in pop_tinst_level if there have been errors since we pushed. */
> + bool had_errors : 1;
>
> /* Count references to this object. If refcount reaches
> refcount_infinity value, we don't increment or decrement the
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index a71705fd085..e8d342f99f6 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -11418,6 +11418,7 @@ push_tinst_level_loc (tree tldcl, tree targs,
> location_t loc)
> new_level->targs = targs;
> new_level->locus = loc;
> new_level->errors = errorcount + sorrycount;
> + new_level->had_errors = false;
> new_level->next = NULL;
> new_level->refcount = 0;
> new_level->path = new_level->visible = nullptr;
> @@ -11468,6 +11469,9 @@ pop_tinst_level (void)
> /* Restore the filename and line number stashed away when we started
> this instantiation. */
> input_location = current_tinst_level->locus;
> + if (unsigned errs = errorcount + sorrycount)
> + if (errs > current_tinst_level->errors)
> + current_tinst_level->had_errors = true;
> set_refcount_ptr (current_tinst_level, current_tinst_level->next);
> --tinst_depth;
> }
> @@ -11487,7 +11491,7 @@ reopen_tinst_level (struct tinst_level *level)
>
> set_refcount_ptr (current_tinst_level, level);
> pop_tinst_level ();
> - if (current_tinst_level)
> + if (current_tinst_level && !current_tinst_level->had_errors)
> current_tinst_level->errors = errorcount+sorrycount;
>
> tree decl = level->maybe_get_node ();
> @@ -28072,7 +28076,9 @@ instantiate_pending_templates (int retries)
> tree instantiation = reopen_tinst_level ((*t)->tinst);
> bool complete = false;
>
> - if (TYPE_P (instantiation))
> + if (limit_bad_template_recursion (instantiation))
> + /* Do nothing. */;
> + else if (TYPE_P (instantiation))
> {
> if (!COMPLETE_TYPE_P (instantiation))
> {
> diff --git a/gcc/testsuite/g++.dg/template/recurse5.C
> b/gcc/testsuite/g++.dg/template/recurse5.C
> new file mode 100644
> index 00000000000..7bfe5239f0a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/recurse5.C
> @@ -0,0 +1,17 @@
> +// Test that we don't bother to instantiate add since there were errors in
> +// checked_add.
> +
> +template <class T> T add (T t) { return t+1; } // { dg-bogus "no match" }
> +
> +template <class T> T checked_add (T t)
> +{
> + add (t);
> + return t+1; // { dg-error "no match" }
> +}
> +
> +struct A { };
> +
> +int main()
> +{
> + checked_add (A());
> +}
>
> base-commit: 6808f74b4f07decb3727624f0e62e7c57ae87022
> --
> 2.49.0
>
