On Thu, 2025-12-11 at 00:00 +0000, Peter Damianov wrote:
> This patch improves -Waddress warnings to be clearer and more
> actionable:
>
> - Use clearer wording: "testing the address of" instead of "the
> address of"
> - Add fix-it hints suggesting to call functions/lambdas with ()
> - Only suggest () for functions/lambdas with no parameters
> - Don't show internal names for lambdas. If the lambda is a variable,
> show the identifier.
>
> Test case:
> bool no_args();
> int with_args(int a, int b);
>
> void test_functions()
> {
> if (no_args) {}
> if (!no_args) {}
> if (with_args) {}
> }
>
> void test_lambdas()
> {
> if ([] { return true; }) {}
>
> auto lambda = [] { return true; };
> if (lambda) {}
>
> auto lambda_with_args = [](int x) { return x > 0; };
> if (lambda_with_args) {}
> }
>
> Before (GCC 15.2.1):
> test.cpp: In function 'void test_functions()':
> test.cpp:6:7: warning: the address of 'bool no_args()' will never
> be NULL [-Waddress]
> 6 | if (no_args) {}
> | ^~~~~~~
> test.cpp:1:6: note: 'bool no_args()' declared here
> 1 | bool no_args();
> | ^~~~~~~
> test.cpp:7:8: warning: the address of 'bool no_args()' will never
> be NULL [-Waddress]
> 7 | if (!no_args) {}
> | ^~~~~~~
> test.cpp:1:6: note: 'bool no_args()' declared here
> 1 | bool no_args();
> | ^~~~~~~
> test.cpp:8:7: warning: the address of 'int with_args(int, int)'
> will never be NULL [-Waddress]
> 8 | if (with_args) {}
> | ^~~~~~~~~
> test.cpp:2:5: note: 'int with_args(int, int)' declared here
> 2 | int with_args(int a, int b);
> | ^~~~~~~~~
> test.cpp: In function 'void test_lambdas()':
> test.cpp:13:7: warning: the address of 'static constexpr bool
> test_lambdas()::<lambda()>::_FUN()' will never be NULL [-Waddress]
> 13 | if ([] { return true; }) {}
> | ^~~~~~~~~~~~~~~~~~~
> test.cpp:13:7: note: 'static constexpr bool
> test_lambdas()::<lambda()>::_FUN()' declared here
> 13 | if ([] { return true; }) {}
> | ^
> test.cpp:16:7: warning: the address of 'static constexpr bool
> test_lambdas()::<lambda()>::_FUN()' will never be NULL [-Waddress]
> 16 | if (lambda) {}
> | ^~~~~~
> test.cpp:15:17: note: 'static constexpr bool
> test_lambdas()::<lambda()>::_FUN()' declared here
> 15 | auto lambda = [] { return true; };
> | ^
> test.cpp:19:7: warning: the address of 'static constexpr bool
> test_lambdas()::<lambda(int)>::_FUN(int)' will never be NULL [-
> Waddress]
> 19 | if (lambda_with_args) {}
> | ^~~~~~~~~~~~~~~~
> test.cpp:18:27: note: 'static constexpr bool
> test_lambdas()::<lambda(int)>::_FUN(int)' declared here
> 18 | auto lambda_with_args = [](int x) { return x > 0; };
> | ^
>
> After (with this patch):
> test.cpp: In function 'void test_functions()':
> test.cpp:6:7: warning: testing the address of 'bool no_args()' will
> always evaluate as 'true' [-Waddress]
> 6 | if (no_args) {}
> | ^~~~~~~
> test.cpp:6:13: note: did you mean to call 'bool no_args()'?
> 6 | if (no_args) {}
> | ~~~~~~^
> | ()
> test.cpp:1:6: note: 'bool no_args()' declared here
> 1 | bool no_args();
> | ^~~~~~~
> test.cpp:7:8: warning: testing the address of 'bool no_args()' will
> always evaluate as 'true' [-Waddress]
> 7 | if (!no_args) {}
> | ^~~~~~~
> test.cpp:7:14: note: did you mean to call 'bool no_args()'?
> 7 | if (!no_args) {}
> | ~~~~~~^
> | ()
> test.cpp:1:6: note: 'bool no_args()' declared here
> 1 | bool no_args();
> | ^~~~~~~
> test.cpp:8:7: warning: testing the address of 'int with_args(int,
> int)' will always evaluate as 'true' [-Waddress]
> 8 | if (with_args) {}
> | ^~~~~~~~~
> test.cpp:2:5: note: 'int with_args(int, int)' declared here
> 2 | int with_args(int a, int b);
> | ^~~~~~~~~
> test.cpp: In function 'void test_lambdas()':
> test.cpp:13:7: warning: testing a lambda expression converted to a
> function pointer will always evaluate as 'true' [-Waddress]
> 13 | if ([] { return true; }) {}
> | ^~~~~~~~~~~~~~~~~~~
> test.cpp:13:25: note: did you mean to call it?
> 13 | if ([] { return true; }) {}
> | ~~~~~~~~~~~~~~~~~~^
> | ()
> test.cpp:16:7: warning: testing 'lambda' converted to a function
> pointer will always evaluate as 'true' [-Waddress]
> 16 | if (lambda) {}
> | ^~~~~~
> test.cpp:16:12: note: did you mean to call 'lambda'?
> 16 | if (lambda) {}
> | ~~~~~^
> | ()
> test.cpp:19:7: warning: testing 'lambda_with_args' converted to a
> function pointer will always evaluate as 'true' [-Waddress]
> 19 | if (lambda_with_args) {}
> | ^~~~~~~~~~~~~~~~
>
> Signed-off-by: Peter Damianov <[email protected]>
> ---
> NOTE: This patch lacks a ChangeLog and tests because I expect it to
> require revision.
>
> gcc/c-family/c-common.cc | 31 ++++++++++++++++--
> gcc/c/c-typeck.cc | 26 ++++++++++++---
> gcc/cp/typeck.cc | 70
> ++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 118 insertions(+), 9 deletions(-)
>
> diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
> index 3cec729c901..35967ecc458 100644
> --- a/gcc/c-family/c-common.cc
> +++ b/gcc/c-family/c-common.cc
> @@ -3662,11 +3662,38 @@ c_common_truthvalue_conversion (location_t
> location, tree expr)
> && !warning_suppressed_p (expr, OPT_Waddress)
> && !warning_suppressed_p (inner, OPT_Waddress))
> {
> + /* Use the location from the ADDR_EXPR if available, as
> it should
> + point to the actual function name usage. */
> + location_t warn_loc = EXPR_HAS_LOCATION (expr)
> + ? EXPR_LOCATION (expr) : location;
> /* Common Ada programmer's mistake. */
> - warning_at (location,
> + warning_at (warn_loc,
> OPT_Waddress,
> - "the address of %qD will always evaluate as
> %<true%>",
> + "testing the address of %qD will always
> evaluate as %<true%>",
> inner);
> + if (TREE_CODE (inner) == FUNCTION_DECL)
> + {
> + tree fntype = TREE_TYPE (inner);
> + tree parms = TYPE_ARG_TYPES (fntype);
> + /* Use the location from the ADDR_EXPR if available,
> as it should
> + point to the actual function name usage. */
> + location_t fn_loc = EXPR_HAS_LOCATION (expr)
> + ? EXPR_LOCATION (expr) : location;
> + location_t start = get_start (fn_loc);
> + location_t finish = get_finish (fn_loc);
> + /* Check if function takes no arguments (parms is
> void_list_node)
> + or takes arguments. */
> + if (parms == void_list_node)
> + {
> + /* Create a location with caret at the end,
> range from start
> + to finish, giving ~~~^ underlining. */
> + location_t insert_loc = make_location (finish,
> start, finish);
> + gcc_rich_location richloc (insert_loc);
> + richloc.add_fixit_insert_after ("()");
> + inform (&richloc,
> + "did you mean to call %qD?", inner);
> + }
> + }
Thanks for the patch; looks promising.
I see you've already spotted some cleanups; here are some more :)
Any time we're adding followup notes to a warning:
(a) we should check the return value of warning_at and guard the notes
with it on warning_at returning true, in case the warning isn't
actually emitted for some reason, so we don't get an "orphan" note
emitted without its parent warning. I *think* there are cases where
the warning_suppressed_p checks could be false (and thus lead to the
warning_at calls), but where the diagnostic is still not emitted e.g.
via pragmas, though am not sure; good to check the return value anyway,
for consistency.
(b) there should be an instance of this RAII class:
auto_diagnostic_group sentinel;
with a lifetime enclosing that of the warning and note(s), so that the
note is grouped "within" the warning (affects SARIF and HTML output).
Sorry about the rather clunky API.
So the code should be something like:
{
.....
auto_diagnostic_group sentinel;
.....
if (warning_at (.......))
inform (......);
......
}
or that conditional could be:
bool warned = warning_at (.....);
......
if (warned)
......
if need be.
These points are pertinent to several places in the patch.
Hope this is constructive.
Dave
> suppress_warning (inner, OPT_Waddress);
> return truthvalue_true_node;
> }
> diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
> index d7c9a324d7a..5e84ab253f2 100644
> --- a/gcc/c/c-typeck.cc
> +++ b/gcc/c/c-typeck.cc
> @@ -14090,7 +14090,7 @@ maybe_warn_for_null_address (location_t loc,
> tree op, tree_code code)
> else
> warning_at (loc, OPT_Waddress,
> "the comparison will always evaluate as %<true%>
> "
> - "for the address of %qE will never be NULL",
> + "because the address of %qE will never be NULL",
> op);
> return;
> }
> @@ -14123,16 +14123,32 @@ maybe_warn_for_null_address (location_t
> loc, tree op, tree_code code)
> if (code == EQ_EXPR)
> w = warning_at (loc, OPT_Waddress,
> "the comparison will always evaluate as
> %<false%> "
> - "for the address of %qE will never be NULL",
> + "because the address of %qE will never be NULL",
> op);
> else
> w = warning_at (loc, OPT_Waddress,
> "the comparison will always evaluate as %<true%>
> "
> - "for the address of %qE will never be NULL",
> + "because the address of %qE will never be NULL",
> op);
>
> - if (w && DECL_P (op))
> - inform (DECL_SOURCE_LOCATION (op), "%qD declared here", op);
> + if (w && TREE_CODE (op) == FUNCTION_DECL)
> + {
> + tree fntype = TREE_TYPE (op);
> + tree parms = TYPE_ARG_TYPES (fntype);
> + location_t start = get_start (loc);
> + location_t finish = get_finish (loc);
> + /* Only suggest adding parentheses if the function takes no
> arguments. */
> + if (parms == void_list_node)
> + {
> + /* Create a location with caret at the end, range from
> start
> + to finish, giving ~~~^ underlining. */
> + location_t insert_loc = make_location (finish, start,
> finish);
> + gcc_rich_location richloc (insert_loc);
> + richloc.add_fixit_insert_after ("()");
> + inform (&richloc, "did you mean to call %qD?", op);
> + }
> + inform (DECL_SOURCE_LOCATION (op), "%qD declared here", op);
> + }
> }
>
> /* Build a binary-operation expression without default conversions.
> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> index 65e08eaa645..bc694a2ddd7 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -5182,9 +5182,75 @@ warn_for_null_address (location_t location,
> tree op, tsubst_flags_t complain)
> || warning_suppressed_p (cop, OPT_Waddress))
> return;
>
> - warned = warning_at (location, OPT_Waddress,
> - "the address of %qD will never be NULL",
> cop);
> + /* Check if this is a lambda static thunk to avoid showing
> ugly internal names. */
> + bool is_lambda = (TREE_CODE (cop) == FUNCTION_DECL
> + && lambda_static_thunk_p (cop));
> + tree var_decl = NULL_TREE;
> +
> + if (is_lambda)
> + {
> + /* For lambda in a variable vs direct lambda expression:
> + - Direct: op is CALL_EXPR with TARGET_EXPR inside
> + - Variable: op is CALL_EXPR with VAR_DECL inside
> + We need to dig into the CALL_EXPR to find the
> VAR_DECL. */
> + tree orig = op;
> + STRIP_NOPS (orig);
> +
> + /* If op is a CALL_EXPR (lambda conversion operator call),
> + check its first argument which is ADDR_EXPR of the
> lambda object. */
> + if (TREE_CODE (orig) == CALL_EXPR && call_expr_nargs
> (orig) > 0)
> + {
> + tree arg0 = CALL_EXPR_ARG (orig, 0);
> + if (TREE_CODE (arg0) == ADDR_EXPR)
> + {
> + tree inner = TREE_OPERAND (arg0, 0);
> + /* Check if it's a VAR_DECL (named variable) or
> TARGET_EXPR (temporary). */
> + if (TREE_CODE (inner) == VAR_DECL && DECL_NAME
> (inner))
> + var_decl = inner;
> + }
> + }
> +
> + if (var_decl)
> + warned = warning_at (location, OPT_Waddress,
> + "testing %qD converted to a
> function pointer "
> + "will always evaluate as %<true%>",
> var_decl);
> + else
> + warned = warning_at (location, OPT_Waddress,
> + "testing a lambda expression
> converted to a "
> + "function pointer will always
> evaluate as %<true%>");
> + }
> + else
> + warned = warning_at (location, OPT_Waddress,
> + "testing the address of %qD will always
> "
> + "evaluate as %<true%>", cop);
> op = cop;
> +
> + /* Add fix-it hints for functions. */
> + if (warned && TREE_CODE (cop) == FUNCTION_DECL)
> + {
> + tree fntype = TREE_TYPE (cop);
> + tree parms = TYPE_ARG_TYPES (fntype);
> + location_t start = get_start (location);
> + location_t finish = get_finish (location);
> + /* Only suggest adding parentheses if the function takes
> no arguments. */
> + if (parms == void_list_node)
> + {
> + /* Create a location with caret at the end, range from
> start
> + to finish, giving ~~~^ underlining. */
> + location_t insert_loc = make_location (finish, start,
> finish);
> + gcc_rich_location richloc (insert_loc);
> + richloc.add_fixit_insert_after ("()");
> + if (is_lambda && var_decl)
> + inform (&richloc, "did you mean to call %qD?",
> var_decl);
> + else if (is_lambda)
> + inform (&richloc, "did you mean to call it?");
> + else
> + inform (&richloc, "did you mean to call %qD?", cop);
> + }
> + if (!is_lambda)
> + inform (DECL_SOURCE_LOCATION (cop), "%qD declared here",
> cop);
> + }
> + return;
> }
> else if (TREE_CODE (cop) == POINTER_PLUS_EXPR)
> {
