[Bug c++/98841] New: wrong ‘operator=’ should return a reference to ‘*this’ [-Weffc++]

[Bug c++/98841] New: wrong ‘operator=’ should return a reference to ‘*this’ [-Weffc++]

[o.mandel at menlosystems dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98841

            Bug ID: 98841
           Summary: wrong ‘operator=’ should return a reference to ‘*this’
                    [-Weffc++]
           Product: gcc
           Version: 10.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: o.mandel at menlosystems dot com
  Target Milestone: ---

Created attachment 50059
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50059&action=edit
Console output of version 8.3.0

I get unexpected warnings about "'operator=' should return a reference to
'*this' [-Weffc++]".

This happens for at least versions 8.3.0 and 10.2.0 of g++. The output of the
following command is attached for both versions:

g++ -v -save-temps -Weffc++ -c t.cpp

In the minimal demo used here this only happens for a template member function,
but in larger code it can also be observed for a plain member function: see
e.g. https://github.com/jbeder/yaml-cpp/issues/970

This might be the same issue as mentioned in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84364#c9 (why the comment asked
for that ticket to be reopened)?

[Bug c++/98841] wrong ‘operator=’ should return a reference to ‘*this’ [-Weffc++]

[o.mandel at menlosystems dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98841

--- Comment #1 from Olaf Mandel <o.mandel at menlosystems dot com> ---
Created attachment 50060
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50060&action=edit
Preprocessed source file of version 8.3.0

[Bug c++/98841] wrong ‘operator=’ should return a reference to ‘*this’ [-Weffc++]

[o.mandel at menlosystems dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98841

--- Comment #2 from Olaf Mandel <o.mandel at menlosystems dot com> ---
Created attachment 50061
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50061&action=edit
Console output of version 10.2.0

[Bug c++/98841] wrong ‘operator=’ should return a reference to ‘*this’ [-Weffc++]

[o.mandel at menlosystems dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98841

--- Comment #3 from Olaf Mandel <o.mandel at menlosystems dot com> ---
Created attachment 50062
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50062&action=edit
Preprocessed source file of version 10.2.0

[Bug c++/98841] wrong ‘operator=’ should return a reference to ‘*this’ [-Weffc++]

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98841

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-01-26
           Keywords|                            |diagnostic
     Ever confirmed|0                           |1
             Blocks|                            |81930
             Status|UNCONFIRMED                 |NEW


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81930
[Bug 81930] [meta-bug] Issues with -Weffc++

[Bug c++/98841] wrong ‘operator=’ should return a reference to ‘*this’ [-Weffc++]

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98841

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
-Weffc++ is really a bad option in general ....

[Bug c++/98841] wrong ‘operator=’ should return a reference to ‘*this’ [-Weffc++]

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98841

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
          /* Returning '*this' is obviously OK.  */
          if (retval == current_class_ref)
            warn = false;
assumes that retval and current_class_ref must be equal, but that isn't clearly
the case, they can be different pointers.
The retval is created in templates here in:
#6  0x0000000000e0a273 in build_min (code=INDIRECT_REF, tt=<record_type
0x7fffea1b4000 Foo>) at ../../gcc/cp/tree.c:3455
#7  0x0000000000e3548c in build_x_indirect_ref (loc=251460, expr=<nop_expr
0x7fffea19ea00>, errorstring=RO_UNARY_STAR, complain=3) at
../../gcc/cp/typeck.c:3329
So, I wonder if we shouldn't do:
          if (retval == current_class_ref
              || operand_equal_p (retval, current_class_ref, 0))
            warn = false;
instead.

[Bug c++/98841] wrong ‘operator=’ should return a reference to ‘*this’ [-Weffc++]

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98841

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 50067
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50067&action=edit
gcc11-pr98841.patch

Untested fix.

[Bug c++/98841] wrong ‘operator=’ should return a reference to ‘*this’ [-Weffc++]

[o.mandel at menlosystems dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98841

--- Comment #7 from Olaf Mandel <o.mandel at menlosystems dot com> ---
(In reply to Olaf Mandel from comment #0)
> In the minimal demo used here this only happens for a template member
> function, but in larger code it can also be observed for a plain member
> function: see e.g. https://github.com/jbeder/yaml-cpp/issues/970
> 
I have to retract that statement: I cannot reproduce this and the two line
numbers in the larger code in question are very similar: 212 and 221. Maybe I
just confused them?

[Bug c++/98841] wrong ‘operator=’ should return a reference to ‘*this’ [-Weffc++]

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98841

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:850a8ec54c4310d779004299bf9a0dec52324e9e

commit r11-6964-g850a8ec54c4310d779004299bf9a0dec52324e9e
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Jan 29 00:39:00 2021 +0100

    c++: Fix -Weffc++ in templates [PR98841]

    We emit a bogus warning on the following testcase, suggesting that the
    operator should return *this even when it does that already.
    The problem is that normally cp_build_indirect_ref_1 ensures that *this
    is folded as current_class_ref, but in templates (if return type is
    non-dependent, otherwise check_return_expr doesn't check it) it didn't
    go through cp_build_indirect_ref_1, but just built another INDIRECT_REF.
    Which means it then doesn't compare pointer-equal to current_class_ref.

    The following patch fixes it by doing in build_x_indirect_ref for
    *this what cp_build_indirect_ref_1 would do.

    2021-01-28  Jakub Jelinek  <jakub@redhat.com>

            PR c++/98841
            * typeck.c (build_x_indirect_ref): For *this, return
current_class_ref.

            * g++.dg/warn/effc5.C: New test.

[Bug c++/98841] wrong ‘operator=’ should return a reference to ‘*this’ [-Weffc++]

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98841

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for GCC 11.

[Bug c++/98841] wrong ‘operator=’ should return a reference to ‘*this’ [-Weffc++]

[o.mandel at menlosystems dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98841

--- Comment #10 from Olaf Mandel <o.mandel at menlosystems dot com> ---
Thank you, that was quick!

Though, didn't this miss the [end of stage 3 development][1] by 11 days?

[1]: <https://gcc.gnu.org/develop.html#timeline>