On Tue, 2025-01-07 at 15:08 -0500, Marek Polacek wrote:
> On Thu, Dec 19, 2024 at 06:40:19PM -0500, David Malcolm wrote:
> > Consider this case of a bad call to a callback function (perhaps
> > due to C23 changing the meaning of () in function decls):
> >
> > struct p {
> > int (*bar)();
> > };
> >
> > void baz() {
> > struct p q;
> > q.bar(1);
> > }
> >
> > Before this patch the C frontend emits:
> >
> > t.c: In function 'baz':
> > t.c:7:5: error: too many arguments to function 'q.bar'
> > 7 | q.bar(1);
> > | ^
> >
> > and the C++ frontend emits:
> >
> > t.c: In function 'void baz()':
> > t.c:7:10: error: too many arguments to function
> > 7 | q.bar(1);
> > | ~~~~~^~~
> >
> > neither of which give the user much help in terms of knowing what
> > was expected, and where the relevant declaration is.
> >
> > With this patch the C frontend emits:
> >
> > t.c: In function 'baz':
> > t.c:7:5: error: too many arguments to function 'q.bar'; expected 0,
> > have 1
> > 7 | q.bar(1);
> > | ^ ~
> > t.c:2:15: note: declared here
> > 2 | int (*bar)();
> > | ^~~
> >
> > (showing the expected vs actual counts, the pertinent field decl,
> > and
> > underlining the first extraneous argument at the callsite)
> >
> > and the C++ frontend emits:
> >
> > t.c: In function 'void baz()':
> > t.c:7:10: error: too many arguments to function; expected 0, have 1
> > 7 | q.bar(1);
> > | ~~~~~^~~
> >
> > (showing the expected vs actual counts; the other data was not
> > accessible
> > without a more invasive patch)
> >
> > Similarly, the patch also updates the "too few arguments" case to
> > also
> > show expected vs actual counts. Doing so requires a tweak to the
> > wording to say "at least" for the case of variadic fns, and for C++
> > fns
> > with default args, where e.g. previously the C FE emitted:
> >
> > s.c: In function 'test':
> > s.c:5:3: error: too few arguments to function 'callee'
> > 5 | callee ();
> > | ^~~~~~
> > s.c:1:6: note: declared here
> > 1 | void callee (const char *, ...);
> > | ^~~~~~
> >
> > with this patch it emits:
> >
> > s.c: In function 'test':
> > s.c:5:3: error: too few arguments to function 'callee'; expected at
> > least 1, have 0
> > 5 | callee ();
> > | ^~~~~~
> > s.c:1:6: note: declared here
> > 1 | void callee (const char *, ...);
> > | ^~~~~~
> >
> > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> > OK for trunk?
> >
> > gcc/c/ChangeLog:
> > PR c/118112
> > * c-typeck.cc (inform_declaration): Add "function_expr"
> > param and
> > use it for cases where we couldn't show the function decl
> > to show
> > field decls for callbacks.
> > (build_function_call_vec): Add missing
> > auto_diagnostic_group.
> > Update for new param of inform_declaration.
> > (convert_arguments): Likewise. For the "too many
> > arguments" case
> > add the expected vs actual counts to the message, and if
> > we have
> > it, add the location_t of the first surplus param as a
> > secondary
> > location within the diagnostic. For the "too few
> > arguments" case,
> > determine the minimum number of arguments required and add
> > the
> > expected vs actual counts to the message, tweaking it to
> > "at least"
> > for variadic functions.
> >
> > gcc/cp/ChangeLog:
> > PR c/118112
> > * typeck.cc (error_args_num): Add params "expected_num",
> > "actual_num", and "at_least_p". Compute "too_many_p" from
> > these
> > rather than have it be a param. Add expected vs actual
> > counts to
> > the messages and tweak them for the "at least" case.
> > (convert_arguments): Update calls to error_args_num to
> > pass in
> > expected vs actual number, and the "at_least_p",
> > determining this
> > for the "too few" case.
> >
> > gcc/testsuite/ChangeLog:
> > PR c/118112
> > * c-c++-common/too-few-arguments.c: New test.
> > * c-c++-common/too-many-arguments.c: New test.
> > * g++.dg/cpp0x/variadic169.C: Verify the reported expected
> > vs
> > actual argument counts.
> > * g++.dg/modules/macloc-1_c.C: Update regexp for addition
> > of param
> > counts to error message.
> > * g++.dg/modules/macloc-1_d.C: Likewise.
> >
> > Signed-off-by: David Malcolm <dmalc...@redhat.com>
>
> [...]
>
> > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> > index 964e549a6122..2966931ca8c1 100644
> > --- a/gcc/cp/typeck.cc
> > +++ b/gcc/cp/typeck.cc
> > @@ -59,7 +59,7 @@ static tree get_delta_difference (tree, tree,
> > bool, bool, tsubst_flags_t);
> > static void casts_away_constness_r (tree *, tree *,
> > tsubst_flags_t);
> > static bool casts_away_constness (tree, tree, tsubst_flags_t);
> > static bool maybe_warn_about_returning_address_of_local (tree,
> > location_t = UNKNOWN_LOCATION);
> > -static void error_args_num (location_t, tree, bool);
> > +static void error_args_num (location_t, tree, int, int, bool);
> > static int convert_arguments (tree, vec<tree, va_gc> **, tree,
> > int,
> > tsubst_flags_t);
> > static bool is_std_move_p (tree);
> > @@ -4533,11 +4533,16 @@ cp_build_function_call_vec (tree function,
> > vec<tree, va_gc> **params,
> > }
> >
> > /* Subroutine of convert_arguments.
> > - Print an error message about a wrong number of arguments. */
> > + Print an error message about a wrong number of arguments.
> > + If AT_LEAST_P is true, then EXPECTED_NUM is the minimum number
> > + of expected arguments. */
>
> Should probably mention ACTUAL_NUM in the comment too.
Done; I've updated that comment to:
/* Subroutine of convert_arguments.
Print an error message about a wrong number of arguments.
If AT_LEAST_P is true, then EXPECTED_NUM is the minimum number
of expected arguments, otherwise EXPECTED_NUM is the exact number
of expected arguments.
ACTUAL_NUM is the actual number of arguments that were explicitly
passed at the callsite (i.e. not counting default arguments). */
>
> >
> > static void
> > -error_args_num (location_t loc, tree fndecl, bool too_many_p)
> > +error_args_num (location_t loc, tree fndecl, int expected_num, int
> > actual_num,
> > + bool at_least_p)
> > {
> > + gcc_assert (expected_num != actual_num);
> > + const bool too_many_p = actual_num > expected_num;
> > if (fndecl)
> > {
> > auto_diagnostic_group d;
> > @@ -4548,22 +4553,28 @@ error_args_num (location_t loc, tree
> > fndecl, bool too_many_p)
> > == DECL_NAME (TYPE_NAME (DECL_CONTEXT
> > (fndecl)))))
> > error_at (loc,
> > too_many_p
> > - ? G_("too many arguments to constructor
> > %q#D")
> > - : G_("too few arguments to constructor
> > %q#D"),
> > - fndecl);
> > + ? G_("too many arguments to constructor
> > %q#D; expected %i, have %i")
> > + : (at_least_p
> > + ? G_("too few arguments to constructor
> > %q#D; expected at least %i, have %i")
> > + : G_("too few arguments to constructor
> > %q#D; expected %i, have %i")),
> > + fndecl, expected_num, actual_num);
> > else
> > error_at (loc,
> > too_many_p
> > - ? G_("too many arguments to member function
> > %q#D")
> > - : G_("too few arguments to member function
> > %q#D"),
> > - fndecl);
> > + ? G_("too many arguments to member function
> > %q#D; expected %i, have %i")
> > + : (at_least_p
> > + ? G_("too few arguments to member
> > function %q#D; expected at least %i, have %i")
> > + : G_("too few arguments to member
> > function %q#D; expected %i, have %i")),
> > + fndecl, expected_num, actual_num);
> > }
>
> Looks like the METHOD_TYPE block is not exercised in our testsuite
> :(.
I'll take a look at adding test coverage for that, I guess. I know
next to nothing about Objective C++ though :(
>
> > else
> > error_at (loc,
> > too_many_p
> > - ? G_("too many arguments to function %q#D")
> > - : G_("too few arguments to function %q#D"),
> > - fndecl);
> > + ? G_("too many arguments to function %q#D;
> > expected %i, have %i")
> > + : (at_least_p
> > + ? G_("too few arguments to function %q#D;
> > expected at least %i, have %i")
> > + : G_("too few arguments to function %q#D;
> > expected %i, have %i")),
> > + fndecl, expected_num, actual_num);
> > if (!DECL_IS_UNDECLARED_BUILTIN (fndecl))
> > inform (DECL_SOURCE_LOCATION (fndecl), "declared here");
> > }
> > @@ -4572,12 +4583,19 @@ error_args_num (location_t loc, tree
> > fndecl, bool too_many_p)
> > if (c_dialect_objc () && objc_message_selector ())
> > error_at (loc,
> > too_many_p
> > - ? G_("too many arguments to method %q#D")
> > - : G_("too few arguments to method %q#D"),
> > - objc_message_selector ());
> > + ? G_("too many arguments to method %q#D;
> > expected %i, have %i")
> > + : (at_least_p
> > + ? G_("too few arguments to method %q#D;
> > expected at least %i, have %i")
> > + : G_("too few arguments to method %q#D;
> > expected %i, have %i")),
> > + objc_message_selector (), expected_num,
> > actual_num);
> > else
> > - error_at (loc, too_many_p ? G_("too many arguments to
> > function")
> > - : G_("too few arguments to
> > function"));
> > + error_at (loc,
> > + too_many_p
> > + ? G_("too many arguments to function; expected
> > %i, have %i")
> > + : (at_least_p
> > + ? G_("too few arguments to function; expected
> > at least %i, have %i")
> > + : G_("too few arguments to function; expected
> > %i, have %i")),
> > + expected_num, actual_num);
> > }
> > }
> >
> > @@ -4607,6 +4625,10 @@ convert_arguments (tree typelist, vec<tree,
> > va_gc> **values, tree fndecl,
> > /* Argument passing is always copy-initialization. */
> > flags |= LOOKUP_ONLYCONVERTING;
> >
> > + /* Preserve actual number of arguments passed (without counting
> > default
> > + args), in case we need to complain about too many/few. */
> > + int actual_num = vec_safe_length (*values);
>
> If we're computing the length here, we don't need to compute it
> again...
>
> > +
> > for (i = 0, typetail = typelist;
> > i < vec_safe_length (*values);
>
> ...here.
Will fix.
>
> > i++)
> > @@ -4621,7 +4643,10 @@ convert_arguments (tree typelist, vec<tree,
> > va_gc> **values, tree fndecl,
> > {
> > if (complain & tf_error)
> > {
> > - error_args_num (input_location, fndecl,
> > /*too_many_p=*/true);
> > + /* Too many args. */
> > + int expected_num = i;
>
> Why not just pass i? error_args_num doesn't modify its args.
I wanted to document the meaning of the argument by giving it a name,
since "i" seems rather nondescript to me. But I can drop that if it
seems weird.
>
> > + error_args_num (input_location, fndecl,
> > expected_num, actual_num,
> > + false);
>
> Nit, but I'd like to have it as /*at_least_p=*/false.
Will fix.
>
> > return i;
> > }
> > else
> > @@ -4743,7 +4768,38 @@ convert_arguments (tree typelist, vec<tree,
> > va_gc> **values, tree fndecl,
> > if (typetail && typetail != void_list_node)
> > {
> > if (complain & tf_error)
> > - error_args_num (input_location, fndecl,
> > /*too_many_p=*/false);
> > + {
> > + /* Not enough args.
> > + Determine minimum number of arguments required.
> > */
> > + int min_expected_num = 0;
> > + bool at_least_p = false;
> > + tree iter = typelist;
> > + while (true)
> > + {
> > + if (!iter)
> > + {
> > + /* Variadic arguments; stop iterating. */
> > + at_least_p = true;
> > + break;
> > + }
> > + if (iter == void_list_node)
> > + /* End of arguments; stop iterating. */
> > + break;
> > + if (fndecl && TREE_PURPOSE (iter)
> > + && TREE_CODE (TREE_PURPOSE (iter)) !=
> > DEFERRED_PARSE)
> > + {
> > + /* Found a default argument; skip this one
> > when
> > + counting minimum required. */
> > + at_least_p = true;
> > + iter = TREE_CHAIN (iter);
> > + continue;
> > + }
> > + ++min_expected_num;
> > + iter = TREE_CHAIN (iter);
> > + }
> > + error_args_num (input_location, fndecl,
> > + min_expected_num, actual_num,
> > at_least_p);
> > + }
> > return -1;
> > }
> > }
>
> I haven't found any issues otherwise.
Thanks; I'm working on an updated patch.
Dave
