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 <[email protected]>
[...]
> 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.
>
> 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 :(.
> 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.
> 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.
> + error_args_num (input_location, fndecl, expected_num, actual_num,
> + false);
Nit, but I'd like to have it as /*at_least_p=*/false.
> 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.
Marek
