* [PATCH] Add fix-it hint for missing return statement in assignment operators (PR c++/85523)
@ 2018-05-01 0:22 David Malcolm
2018-05-01 0:25 ` David Malcolm
2018-05-01 11:18 ` Nathan Sidwell
0 siblings, 2 replies; 5+ messages in thread
From: David Malcolm @ 2018-05-01 0:22 UTC (permalink / raw)
To: gcc-patches, Jonathan Wakely, Nathan Sidwell; +Cc: Andrew Haley, David Malcolm
Following on from the thread on the "gcc" list here:
https://gcc.gnu.org/ml/gcc/2018-04/msg00172.html
here's an updated version of Jonathan's patch, which:
* eliminates the separate "note" in favor of simplying having
the warning itself emit the "return *this;" fix-it hint
pr85523.C: In member function 's1& s1::operator=(const s1&)':
pr85523.C:7:30: warning: no return statement in function returning
non-void [-Wreturn-type]
s1& operator=(const s1&) { }
^
return *this;
In doing so I guarded it with a call to:
global_dc->option_enabled (OPT_Wreturn_type
as per the insides of diagnostic.s, since adding a fix-it hint to
a rich_location is non-trivial (e.g. it requires an allocation).
* uses the new gcc_rich_location::add_fixit_insert_formatted to
make the fix-it hint respect typical C++ code formatting
conventions
* adds testcases
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
OK for trunk?
Note for ChangeLog: jwakely is co-author.
gcc/cp/ChangeLog:
PR c++/85523
* decl.c: Include "gcc-rich-location.h".
(add_return_star_this_fixit): New function.
(finish_function): When warning about missing return statements in
functions returning non-void, add a "return *this;" fix-it hint for
assignment operators.
gcc/testsuite/ChangeLog:
PR c++/85523
* g++.dg/pr85523.C: New test.
---
gcc/cp/decl.c | 34 +++++++++++++++-
gcc/testsuite/g++.dg/pr85523.C | 88 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 120 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/pr85523.C
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 07f3a61..7952106 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -51,6 +51,7 @@ along with GCC; see the file COPYING3. If not see
#include "builtins.h"
#include "gimplify.h"
#include "asan.h"
+#include "gcc-rich-location.h"
/* Possible cases of bad specifiers type used by bad_specifiers. */
enum bad_spec_place {
@@ -15677,6 +15678,22 @@ maybe_save_function_definition (tree fun)
register_constexpr_fundef (fun, DECL_SAVED_TREE (fun));
}
+/* Attempt to add a fix-it hint to RICHLOC suggesting the insertion
+ of "return *this;" immediately before its location, using FNDECL's
+ first statement (if any) to give the indentation, if appropriate. */
+
+static void
+add_return_star_this_fixit (gcc_rich_location *richloc, tree fndecl)
+{
+ location_t indent = UNKNOWN_LOCATION;
+ tree stmts = expr_first (DECL_SAVED_TREE (fndecl));
+ if (stmts)
+ indent = EXPR_LOCATION (stmts);
+ richloc->add_fixit_insert_formatted ("return *this;",
+ richloc->get_loc (),
+ indent);
+}
+
/* Finish up a function declaration and compile that function
all the way to assembler language output. The free the storage
for the function definition. INLINE_P is TRUE if we just
@@ -15870,8 +15887,21 @@ finish_function (bool inline_p)
&& !DECL_DESTRUCTOR_P (fndecl)
&& targetm.warn_func_return (fndecl))
{
- warning (OPT_Wreturn_type,
- "no return statement in function returning non-void");
+ gcc_rich_location richloc (input_location);
+ /* Potentially add a "return *this;" fix-it hint for
+ assignment operators. */
+ if (IDENTIFIER_ASSIGN_OP_P (DECL_NAME (fndecl)))
+ {
+ tree valtype = TREE_TYPE (DECL_RESULT (fndecl));
+ if (TREE_CODE (valtype) == REFERENCE_TYPE
+ && same_type_ignoring_top_level_qualifiers_p
+ (TREE_TYPE (valtype), TREE_TYPE (current_class_ref)))
+ if (global_dc->option_enabled (OPT_Wreturn_type,
+ global_dc->option_state))
+ add_return_star_this_fixit (&richloc, fndecl);
+ }
+ warning_at (&richloc, OPT_Wreturn_type,
+ "no return statement in function returning non-void");
TREE_NO_WARNING (fndecl) = 1;
}
diff --git a/gcc/testsuite/g++.dg/pr85523.C b/gcc/testsuite/g++.dg/pr85523.C
new file mode 100644
index 0000000..9cd939b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr85523.C
@@ -0,0 +1,88 @@
+/* { dg-options "-fdiagnostics-show-caret" } */
+
+/* Verify that we emit a "return *this;" fix-it hint for
+ a missing return in an assignment operator. */
+
+struct s1 {
+ s1& operator=(const s1&) { } // { dg-warning "no return statement in function returning non-void" }
+ /* { dg-begin-multiline-output "" }
+ s1& operator=(const s1&) { }
+ ^
+ return *this;
+ { dg-end-multiline-output "" } */
+};
+
+/* Likewise for +=. */
+
+struct s2 {
+ s2& operator+=(const s2&) {} // { dg-warning "no return statement in function returning non-void" }
+ /* { dg-begin-multiline-output "" }
+ s2& operator+=(const s2&) {}
+ ^
+ return *this;
+ { dg-end-multiline-output "" } */
+};
+
+/* No warning for "void" return. */
+
+struct s3 {
+ void operator=(const s3&) { }
+};
+
+/* We shouldn't issue the fix-it hint if the return type isn't right. */
+
+struct s4 {
+ int operator=(int) { } // { dg-warning "no return statement in function returning non-void" }
+ /* { dg-begin-multiline-output "" }
+ int operator=(int) { }
+ ^
+ { dg-end-multiline-output "" } */
+};
+
+/* Example of a multi-line fix-it hint. */
+
+struct s5 {
+ int i;
+ s5& operator=(const s5& z) {
+ i = z.i;
+ } // { dg-warning "no return statement in function returning non-void" }
+ /* { dg-begin-multiline-output "" }
++ return *this;
+ }
+ ^
+ { dg-end-multiline-output "" } */
+};
+
+/* Example of a multi-line fix-it hint with other statements. */
+
+extern void log (const char *);
+struct s6 {
+ int i;
+ s6& operator=(const s6& z) {
+ log ("operator=\n");
+ i = z.i;
+ } // { dg-warning "no return statement in function returning non-void" }
+ /* { dg-begin-multiline-output "" }
++ return *this;
+ }
+ ^
+ { dg-end-multiline-output "" } */
+};
+
+/* Another example of a multi-line fix-it hint with other statements. */
+
+struct s7 {
+ int i;
+ s7& operator=(const s6& z) {
+ if (z.i)
+ log ("operator=\n");
+ else
+ log ("operator=\n");
+ i = z.i;
+ } // { dg-warning "no return statement in function returning non-void" }
+ /* { dg-begin-multiline-output "" }
++ return *this;
+ }
+ ^
+ { dg-end-multiline-output "" } */
+};
--
1.8.5.3
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Add fix-it hint for missing return statement in assignment operators (PR c++/85523)
2018-05-01 0:22 [PATCH] Add fix-it hint for missing return statement in assignment operators (PR c++/85523) David Malcolm
@ 2018-05-01 0:25 ` David Malcolm
2018-05-01 11:18 ` Nathan Sidwell
1 sibling, 0 replies; 5+ messages in thread
From: David Malcolm @ 2018-05-01 0:25 UTC (permalink / raw)
To: gcc-patches, Jonathan Wakely, Nathan Sidwell; +Cc: Andrew Haley
On Mon, 2018-04-30 at 20:29 -0400, David Malcolm wrote:
[...]
> In doing so I guarded it with a call to:
> global_dc->option_enabled (OPT_Wreturn_type
Gah; typo, sorry:
> as per the insides of diagnostic.s, since adding a fix-it hint to
~~~~~~~~~~~^
diagnostic.c
> a rich_location is non-trivial (e.g. it requires an allocation).
[...]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Add fix-it hint for missing return statement in assignment operators (PR c++/85523)
2018-05-01 0:22 [PATCH] Add fix-it hint for missing return statement in assignment operators (PR c++/85523) David Malcolm
2018-05-01 0:25 ` David Malcolm
@ 2018-05-01 11:18 ` Nathan Sidwell
2018-08-03 18:41 ` David Malcolm
1 sibling, 1 reply; 5+ messages in thread
From: Nathan Sidwell @ 2018-05-01 11:18 UTC (permalink / raw)
To: David Malcolm, gcc-patches, Jonathan Wakely; +Cc: Andrew Haley
On 04/30/2018 08:29 PM, David Malcolm wrote:
> Following on from the thread on the "gcc" list here:
>
> https://gcc.gnu.org/ml/gcc/2018-04/msg00172.html
>
> here's an updated version of Jonathan's patch, which:
> + {
> + tree valtype = TREE_TYPE (DECL_RESULT (fndecl));
> + if (TREE_CODE (valtype) == REFERENCE_TYPE
> + && same_type_ignoring_top_level_qualifiers_p
> + (TREE_TYPE (valtype), TREE_TYPE (current_class_ref)))
While this is probably close enough, you could
*) use convert_to_base to include cases where the return type's a base
of the current object.
*) convert_to_base would also allow dropping the REFERENCE_TYPE check
here, so icky code returning by-value could also be caught.
Up to you. But otherwise ok.
nathan
--
Nathan Sidwell
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Add fix-it hint for missing return statement in assignment operators (PR c++/85523)
2018-05-01 11:18 ` Nathan Sidwell
@ 2018-08-03 18:41 ` David Malcolm
2018-08-07 1:45 ` H.J. Lu
0 siblings, 1 reply; 5+ messages in thread
From: David Malcolm @ 2018-08-03 18:41 UTC (permalink / raw)
To: Nathan Sidwell, gcc-patches, Jonathan Wakely; +Cc: Andrew Haley
On Tue, 2018-05-01 at 07:18 -0400, Nathan Sidwell wrote:
> On 04/30/2018 08:29 PM, David Malcolm wrote:
> > Following on from the thread on the "gcc" list here:
> >
> > https://gcc.gnu.org/ml/gcc/2018-04/msg00172.html
> >
> > here's an updated version of Jonathan's patch, which:
> > + {
> > + tree valtype = TREE_TYPE (DECL_RESULT (fndecl));
> > + if (TREE_CODE (valtype) == REFERENCE_TYPE
> > + && same_type_ignoring_top_level_qualifiers_p
> > + (TREE_TYPE (valtype), TREE_TYPE
> > (current_class_ref)))
>
> While this is probably close enough, you could
> *) use convert_to_base to include cases where the return type's a
> base
> of the current object.
> *) convert_to_base would also allow dropping the REFERENCE_TYPE
> check
> here, so icky code returning by-value could also be caught.
>
> Up to you. But otherwise ok.
Sorry about the belated response; this fell off my radar for some
reason.
I looked at updating it to support the cases you suggest, but I wasn't
happy with issuing the fix-it hint for them, so I've gone with the
patch as-is.
Committed to trunk as r263298 (after rebasing and re-testing)
Thanks
Dave
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Add fix-it hint for missing return statement in assignment operators (PR c++/85523)
2018-08-03 18:41 ` David Malcolm
@ 2018-08-07 1:45 ` H.J. Lu
0 siblings, 0 replies; 5+ messages in thread
From: H.J. Lu @ 2018-08-07 1:45 UTC (permalink / raw)
To: David Malcolm; +Cc: Nathan Sidwell, GCC Patches, Jonathan Wakely, Andrew Haley
On Fri, Aug 3, 2018 at 11:41 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Tue, 2018-05-01 at 07:18 -0400, Nathan Sidwell wrote:
>> On 04/30/2018 08:29 PM, David Malcolm wrote:
>> > Following on from the thread on the "gcc" list here:
>> >
>> > https://gcc.gnu.org/ml/gcc/2018-04/msg00172.html
>> >
>> > here's an updated version of Jonathan's patch, which:
>> > + {
>> > + tree valtype = TREE_TYPE (DECL_RESULT (fndecl));
>> > + if (TREE_CODE (valtype) == REFERENCE_TYPE
>> > + && same_type_ignoring_top_level_qualifiers_p
>> > + (TREE_TYPE (valtype), TREE_TYPE
>> > (current_class_ref)))
>>
>> While this is probably close enough, you could
>> *) use convert_to_base to include cases where the return type's a
>> base
>> of the current object.
>> *) convert_to_base would also allow dropping the REFERENCE_TYPE
>> check
>> here, so icky code returning by-value could also be caught.
>>
>> Up to you. But otherwise ok.
>
> Sorry about the belated response; this fell off my radar for some
> reason.
>
> I looked at updating it to support the cases you suggest, but I wasn't
> happy with issuing the fix-it hint for them, so I've gone with the
> patch as-is.
>
> Committed to trunk as r263298 (after rebasing and re-testing)
>
This caused:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86872
--
H.J.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-08-07 1:45 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-01 0:22 [PATCH] Add fix-it hint for missing return statement in assignment operators (PR c++/85523) David Malcolm
2018-05-01 0:25 ` David Malcolm
2018-05-01 11:18 ` Nathan Sidwell
2018-08-03 18:41 ` David Malcolm
2018-08-07 1:45 ` H.J. Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).