From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id B229A3858427 for ; Fri, 26 Aug 2022 17:04:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B229A3858427 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1661533457; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=lxjcDwWhfwQXhBCXOBo7LOKSdGAYueS5brK7yTKrBb8=; b=UA0mXveTYD4lP1Mmepn8KPlC1osqxhdij3krLUPf1AWNQXFLs/gWqnHH4MiUKm6hLnbYaF rk+ejm6Kq8Xfe6gEhtp7eWXo3j/GBKg44Rjz9bBaBWGq4WFqjXE4Iuvl/98MU36+NwoSxs WDOdEXhNnXL2JihgN9871pEthQxRFHk= Received: from mail-qv1-f71.google.com (mail-qv1-f71.google.com [209.85.219.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-434-eQVUSb56MFyyTIeWi9QCzw-1; Fri, 26 Aug 2022 13:04:14 -0400 X-MC-Unique: eQVUSb56MFyyTIeWi9QCzw-1 Received: by mail-qv1-f71.google.com with SMTP id og5-20020a056214428500b00496b5246db5so1365722qvb.10 for ; Fri, 26 Aug 2022 10:04:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc; bh=Pamj/czBdphjD0BG5l+QS6Nk9R649SAJmZ7AC54MjL0=; b=4wV/4NDfSdV+4Kr+DDdYr7Ob3cigwDr/bwUyp7d0K8a4PfN/R8vkT5hwP5sPb2YNVv biO8ijEjTEK7y91LDORoFP8gxqqzM4z0Kfp8PAUapKWHG6ECZDI6Zeun4ZMCbaN6gcDU n1MmFrszpUcojiDmjXKagUieI2BkhL69BZbBoV1i7EQBdqBZXweCWJr5P6ZpyHUs6i3f AKUCR6wWEQotad6rIOoD4MM0nYVK1MVih6tfaZs4QeLtVrCwT6NmZ0EeMqMvYhVwSfH7 NFimrNe8x8OureIaF4ejtZnotqMAEOqNh/lIqiBqaVlX7NX+ByMHoqU0ICr0yC26Mf9k itKQ== X-Gm-Message-State: ACgBeo1KZfBXxojRIeQVpkS9C8lfm8yQf/btYEO5iCK4tFFWKDlCNXX4 gXqbGuw+ffZArQdrn9vrBORte2gOvKA0FVnvsUysEsuar+0wD3U9t54vEJ6k4siFPizCf/nrMI6 tAQRSB5HtTPJO/afW3w== X-Received: by 2002:a05:622a:292:b0:344:616c:25c8 with SMTP id z18-20020a05622a029200b00344616c25c8mr598286qtw.428.1661533453221; Fri, 26 Aug 2022 10:04:13 -0700 (PDT) X-Google-Smtp-Source: AA6agR48F+5+TWpKlmqmq78cPmjwFtCRS0DQR0sbXKRtE/HYPnJ6wciqtwgL6m61hyT9uD7Oufv/qw== X-Received: by 2002:a05:622a:292:b0:344:616c:25c8 with SMTP id z18-20020a05622a029200b00344616c25c8mr598264qtw.428.1661533452826; Fri, 26 Aug 2022 10:04:12 -0700 (PDT) Received: from redhat.com ([2601:184:4780:4310::e531]) by smtp.gmail.com with ESMTPSA id x22-20020a05620a449600b006af10bd3635sm125599qkp.57.2022.08.26.10.04.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 26 Aug 2022 10:04:11 -0700 (PDT) Date: Fri, 26 Aug 2022 13:04:09 -0400 From: Marek Polacek To: Jason Merrill Cc: GCC Patches Subject: [PATCH v5] c++: Implement -Wself-move warning [PR81159] Message-ID: References: <3e9dea44-1e0e-e329-575a-aa621d19deeb@redhat.com> <9e980813-7ab7-25dc-7f74-56faaafafb84@redhat.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/2.2.6 (2022-06-05) X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-22.8 required=5.0 tests=BAYES_00,DKIM_INVALID,DKIM_SIGNED,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Thu, Aug 25, 2022 at 08:52:58PM -0400, Jason Merrill wrote: > On 8/25/22 17:49, Marek Polacek wrote: > > On Thu, Aug 25, 2022 at 09:25:43AM -0400, Jason Merrill wrote: > > > On 8/24/22 17:30, Marek Polacek wrote: > > > > On Tue, Aug 23, 2022 at 05:27:00PM -0400, Jason Merrill wrote: > > > > > On 8/23/22 09:39, Marek Polacek wrote: > > > > > > + tree arg = CALL_EXPR_ARG (fn, 0); > > > > > > + extract_op (arg); > > > > > > + if (TREE_CODE (arg) == ADDR_EXPR) > > > > > > + arg = TREE_OPERAND (arg, 0); > > > > > > + tree type = TREE_TYPE (lhs); > > > > > > + lhs = maybe_undo_parenthesized_ref (lhs); > > > > > > + STRIP_ANY_LOCATION_WRAPPER (lhs); > > > > > > + const bool print_var_p = (DECL_P (lhs) > > > > > > + || REFERENCE_REF_P (lhs) > > > > > > + || TREE_CODE (lhs) == COMPONENT_REF); > > > > > > > > > > Why include REFERENCE_REF_P and COMPONENT_REF? Reference refs should be > > > > > stripped before this test, member refs aren't variables. > > > > > > > > I'm checking REFERENCE_REF_P and COMPONENT_REF to say "moving a variable" > > > > in #1 and #3. The REFERENCE_REF_P check means that we also say "variable" > > > > for #2. Sure, "A variable is introduced by the declaration of a reference > > > > other than a non-static data member", but I'm not sure if users care about > > > > that here? > > > > > > > > If I strip REFERENCE_REFs before the check then the result will be the > > > > same. > > > > > > That's what I was suggesting, yes: Strip the REFERENCE_REF so DECL_P can see > > > the decl. > > > > Ok, I've added the REFERENCE_REF stripping. But I've still left the > > COMPONENT_REF in. Perhaps we could say "moving a member" to itself for > > COMPONENT_REFs. Or just say "moving 'x' of type 'int' to itself" and > > avoid all of this. :) > > Sure, that would be simpler. In any case, we shouldn't call member > references variables. Here's the "moving 'x' of type 'int' to itself" version then: Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- About 5 years ago we got a request to implement -Wself-move, which warns about useless moves like this: int x; x = std::move (x); This patch implements that warning. PR c++/81159 gcc/c-family/ChangeLog: * c.opt (Wself-move): New option. gcc/cp/ChangeLog: * typeck.cc (maybe_warn_self_move): New. (cp_build_modify_expr): Call maybe_warn_self_move. gcc/ChangeLog: * doc/invoke.texi: Document -Wself-move. gcc/testsuite/ChangeLog: * g++.dg/warn/Wself-move1.C: New test. --- gcc/c-family/c.opt | 4 + gcc/cp/typeck.cc | 53 +++++++++- gcc/doc/invoke.texi | 23 ++++- gcc/testsuite/g++.dg/warn/Wself-move1.C | 125 ++++++++++++++++++++++++ 4 files changed, 203 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move1.C diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index dfdebd596ef..f776efd39d8 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -1229,6 +1229,10 @@ Wselector ObjC ObjC++ Var(warn_selector) Warning Warn if a selector has multiple methods. +Wself-move +C++ ObjC++ Var(warn_self_move) Warning LangEnabledBy(C++ ObjC++, Wall) +Warn when a value is moved to itself with std::move. + Wsequence-point C ObjC C++ ObjC++ Var(warn_sequence_point) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Warn about possible violations of sequence point rules. diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index 7fde65adaa4..b99947c10fd 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -8897,7 +8897,56 @@ cp_build_c_cast (location_t loc, tree type, tree expr, return error_mark_node; } - + +/* Warn when a value is moved to itself with std::move. LHS is the target, + RHS may be the std::move call, and LOC is the location of the whole + assignment. */ + +static void +maybe_warn_self_move (location_t loc, tree lhs, tree rhs) +{ + if (!warn_self_move) + return; + + /* C++98 doesn't know move. */ + if (cxx_dialect < cxx11) + return; + + if (processing_template_decl) + return; + + if (!REFERENCE_REF_P (rhs) + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) + return; + tree fn = TREE_OPERAND (rhs, 0); + if (!is_std_move_p (fn)) + return; + + /* Just a little helper to strip * and various NOPs. */ + auto extract_op = [] (tree &op) { + STRIP_NOPS (op); + while (INDIRECT_REF_P (op)) + op = TREE_OPERAND (op, 0); + op = maybe_undo_parenthesized_ref (op); + STRIP_ANY_LOCATION_WRAPPER (op); + }; + + tree arg = CALL_EXPR_ARG (fn, 0); + extract_op (arg); + if (TREE_CODE (arg) == ADDR_EXPR) + arg = TREE_OPERAND (arg, 0); + tree type = TREE_TYPE (lhs); + tree orig_lhs = lhs; + extract_op (lhs); + if (cp_tree_equal (lhs, arg)) + { + auto_diagnostic_group d; + if (warning_at (loc, OPT_Wself_move, + "moving %qE of type %qT to itself", orig_lhs, type)) + inform (loc, "remove % call"); + } +} + /* For use from the C common bits. */ tree build_modify_expr (location_t location, @@ -9101,6 +9150,8 @@ cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode, if (modifycode == NOP_EXPR) { + maybe_warn_self_move (loc, lhs, rhs); + if (c_dialect_objc ()) { result = objc_maybe_build_modify_expr (lhs, rhs); diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 9ba83a6b1f3..6131bfa7acf 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -264,7 +264,7 @@ in the following sections. -Wreorder -Wregister @gol -Wstrict-null-sentinel -Wno-subobject-linkage -Wtemplates @gol -Wno-non-template-friend -Wold-style-cast @gol --Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol +-Woverloaded-virtual -Wno-pmf-conversions -Wself-move -Wsign-promo @gol -Wsized-deallocation -Wsuggest-final-methods @gol -Wsuggest-final-types -Wsuggest-override @gol -Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol @@ -5838,6 +5838,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wreorder @gol -Wrestrict @gol -Wreturn-type @gol +-Wself-move @r{(only for C++)} @gol -Wsequence-point @gol -Wsign-compare @r{(only in C++)} @gol -Wsizeof-array-div @gol @@ -6823,6 +6824,26 @@ of a declaration: This warning is enabled by @option{-Wall}. +@item -Wno-self-move @r{(C++ and Objective-C++ only)} +@opindex Wself-move +@opindex Wno-self-move +This warning warns when a value is moved to itself with @code{std::move}. +Such a @code{std::move} typically has no effect. + +@smallexample +struct T @{ +@dots{} +@}; +void fn() +@{ + T t; + @dots{} + t = std::move (t); +@} +@end smallexample + +This warning is enabled by @option{-Wall}. + @item -Wsequence-point @opindex Wsequence-point @opindex Wno-sequence-point diff --git a/gcc/testsuite/g++.dg/warn/Wself-move1.C b/gcc/testsuite/g++.dg/warn/Wself-move1.C new file mode 100644 index 00000000000..5c9fc92c347 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wself-move1.C @@ -0,0 +1,125 @@ +// PR c++/81159 +// { dg-do compile { target c++11 } } +// { dg-options "-Wself-move" } + +// Define std::move. +namespace std { + template + struct remove_reference + { typedef _Tp type; }; + + template + struct remove_reference<_Tp&> + { typedef _Tp type; }; + + template + struct remove_reference<_Tp&&> + { typedef _Tp type; }; + + template + constexpr typename std::remove_reference<_Tp>::type&& + move(_Tp&& __t) noexcept + { return static_cast::type&&>(__t); } +} + +int g; + +struct S { + int x; + S(S&& o) { + x = std::move (x); // { dg-warning "moving '\[^\n\r]*S::x' of type .int. to itself" } + x = std::move (o.x); + o.x = std::move (x); + o.x = std::move (o.x); // { dg-warning "moving 'o.S::x' of type .int. to itself" } + } + void foo (int x) { + x = std::move (x); // { dg-warning "moving 'x' of type .int. to itself" } + } +}; + +struct X { + int x; + X(int x) : x(std::move (x)) { } +}; + +struct A {}; +struct B { A a; }; +struct C { C(); ~C(); }; +struct D { D(); D(const D&); D(D&&); D& operator=(const D&); }; + +void +test () +{ + int i = 42; + i = std::move (i); // { dg-warning "moving 'i' of type .int. to itself" } + (i) = std::move (i); // { dg-warning "moving 'i' of type .int. to itself" } + + g = std::move (g); // { dg-warning "moving 'g' of type .int. to itself" } + (g) = std::move (g); // { dg-warning "moving 'g' of type .int. to itself" } + + A a; + a = std::move (a); // { dg-warning "moving 'a' of type .A. to itself" } + (a) = std::move (a); // { dg-warning "moving 'a' of type .A. to itself" } + + B b; + b = std::move (b); // { dg-warning "moving 'b' of type .B. to itself" } + (b) = std::move (b); // { dg-warning "moving 'b' of type .B. to itself" } + b.a = std::move (b.a); // { dg-warning "moving 'b.B::a' of type .A. to itself" } + (b.a) = std::move (b.a); // { dg-warning "moving 'b.B::a' of type .A. to itself" } + + C c; + c = std::move (c); // { dg-warning "moving 'c' of type .C. to itself" } + D d; + d = std::move (d); // { dg-warning "moving 'd' of type .D. to itself" } +} + +template +void ttest () +{ + T t; + t = std::move (t); // { dg-warning "moving 't' of type .A. to itself" } +} + +template void ttest(); + +void +testref (int &r, int &&rr) +{ + r = std::move (r); // { dg-warning "moving 'r' of type .int. to itself" } + rr = std::move (rr); // { dg-warning "moving 'rr' of type .int. to itself" } +} + +// Test various other arguments to std::move. +template +void +testargs (T *Tptr, T **Tpptr, T& Tref, T&& Trref, const T *Tcptr) +{ + Tptr = std::move (Tptr); // { dg-warning "moving 'Tptr' of type 'int\\*' to itself" } + *Tptr = std::move (*Tptr); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } + *Tptr = std::move (*(Tptr)); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } + *(Tptr) = std::move (*Tptr); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } + *(Tptr + 1) = std::move (*(Tptr + 1)); // { dg-warning "moving '\[^\n\r]*Tptr\[^\n\r]*' of type 'int' to itself" } + *(Tptr + 1) = std::move (*(Tptr + 2)); + (*(Tptr)) = std::move (*Tptr); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } + *Tpptr = std::move (*Tpptr); // { dg-warning "moving '\\* Tpptr' of type 'int\\*' to itself" } + **Tpptr = std::move (**Tpptr); // { dg-warning "moving '\\* \\* Tpptr' of type 'int' to itself" } + Tref = std::move (Tref); // { dg-warning "moving 'Tref' of type 'int' to itself" } + Trref = std::move (Trref); // { dg-warning "moving 'Trref' of type 'int' to itself" } + Tcptr = std::move (Tcptr); // { dg-warning "moving 'Tcptr' of type 'const int\\*' to itself" } + (Tptr) = std::move (Tptr); // { dg-warning "moving 'Tptr' of type 'int\\*' to itself" } + (*Tptr) = std::move (*Tptr); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } + (*Tpptr) = std::move (*Tpptr); // { dg-warning "moving '\\* Tpptr' of type 'int\\*' to itself" } + (**Tpptr) = std::move (**Tpptr); // { dg-warning "moving '\\* \\* Tpptr' of type 'int' to itself" } + (*(*(Tpptr))) = std::move (**Tpptr); // { dg-warning "moving '\\* \\* Tpptr' of type 'int' to itself" } + (Tref) = std::move (Tref); // { dg-warning "moving 'Tref' of type 'int' to itself" } + (Trref) = std::move (Trref); // { dg-warning "moving 'Trref' of type 'int' to itself" } + (Tcptr) = std::move (Tcptr); // { dg-warning "moving 'Tcptr' of type 'const int\\*' to itself" } +} + +void +call_testargs () +{ + int i = 42; + int *p = &i; + testargs(&i, &p, i, 42, &i); +} base-commit: 1e2462890ac748a5e5b6a34fdeb61e7027863a90 -- 2.37.2