llvm dev - Sep 2018 - [cfe-dev] New warnings when building trunk with GCC 9

+ Erik, who implemented DR1579

Originally, I had the warning similar to GCC's warning, but took it out due
to not having DR1579 implemented in clang (warning changed in r243594)

Erik in r274291 implemented DR1579, although PR27785 didn't mention
anything about std::move

It looks like what's happening is that Clang and GCC handles the return
differently.  Clang needs the std::move call to use the move constructor
while GCC will use the move constructor with or without the std::move
call.  This means that the warning is currently correct when running on
either compiler.

This is a reduced example.  Compiled with Clang, it will print "move
constructor" then "copy constructor".  GCC will print "move
constructor"
twice.

#include <iostream>
#include <memory>

struct A {
  A(int) {}
  A() {  std::cout << "empty constructor"; }
  A(const A&) {  std::cout << "copy constructor\n"; }
  A(A&&) {  std::cout << "move constructor\n"; }
};
struct B {
  B(A a) {}
};

A getA() { return A(1); }

B run1() {
  A a = getA();
  return std::move(a);
}

B run2() {
  A a = getA();
  return a;
}

int main() {
  std::cout << "with move:\n";
  run1();
  std::cout << "no move:\n";
  run2();
}



On Tue, Sep 18, 2018 at 4:07 PM Richard Trieu <rtrieu at google.com>
wrote:
> I'll look into the pessimizing move warning, then after that, the
> deprecated copy warning.
>
> On Sun, Sep 16, 2018 at 10:41 AM Richard Smith <richard at
metafoo.co.uk>
> wrote:
>
>> Yes, we should produce this warning in C++11 mode too. (I could be
>> misrecalling, but I think the rationale for the current behaviour is
based
>> on historical GCC behaviour.)
>>
>> On Sun, 16 Sep 2018, 10:04 David Blaikie via cfe-dev, <
>> cfe-dev at lists.llvm.org> wrote:
>>
>>> Fair point made on that thread - that this is a DR, so technically
the
>>> std::move is pessimizing even in C++11 mode. Richard: Any thoughts
on
>>> generalizing the warning to cover these cases even in C++11 mode?
>>>
>>> On Sat, Sep 15, 2018 at 2:37 AM Dávid Bolvanský <
>>> david.bolvansky at gmail.com> wrote:
>>>
>>>> There is a new discussion related to -Wredundant-move warning
on GCC
>>>> bugzilla.
>>>>
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87300
>>>>
>>>> pi 14. 9. 2018 o 9:53 David Blaikie <dblaikie at
gmail.com> napísal(a):
>>>>
>>>>>
>>>>>
>>>>> On Fri, Sep 14, 2018 at 12:48 AM Stephan Bergmann
<sbergman at redhat.com>
>>>>> wrote:
>>>>>
>>>>>> On 13/09/2018 18:22, David Blaikie via llvm-dev wrote:
>>>>>> > On Thu, Sep 13, 2018 at 12:13 AM Dávid Bolvanský
via llvm-dev
>>>>>> > <llvm-dev at lists.llvm.org <mailto:llvm-dev
at lists.llvm.org>> wrote:
>>>>>> >
>>>>>> 
/home/davidbolvansky/trunk/llvm/unittests/ExecutionEngine/Orc/CompileOnDemandLayerTest.cpp:79:40:
>>>>>>
>>>>>> >       required from here
>>>>>> >
>>>>>> 
/home/davidbolvansky/trunk/llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h:314:29:
>>>>>> >     warning: redundant move in return statement
[-Wredundant-move]
>>>>>> >     314 |         return std::move(Err);
>>>>>>
>>>>>> Note that the move (into the implicit JITSymbol(Error)
ctor) is only
>>>>>> redundant if the compiler implements a fix for
>>>>>>
<http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579>
>>>>>> "Return by converting move constructor". 
(But not sure whether the
>>>>>> LLVM
>>>>>> compiler baselines imply that, anyway.
>>>>>
>>>>>
>>>>> Looks like that was accepted for C++14, not 11. So I
don't believe
>>>>> it's valid to remove the std::move in C++11 code.
(& I believe Clang's
>>>>> warning correctly diagnoses this only in the right language
versions) - so
>>>>> we probably want to disable the buggy GCC warning here,
then.
>>>>>
>>>>>
>>>>>> In LibreOffice it forced me to
>>>>>> introduce ugly #ifs, to not have to disable that
warning outright,
>>>>>> <
>>>>>>
https://cgit.freedesktop.org/libreoffice/core/commit/?id=dc06c8f4989fc28d0c31ebd333e53dfe0e0f5f66>
>>>>>>
>>>>>> "-Werror=redundant-move (GCC 9), take two".)
>>>>>>
>>>>> _______________________________________________
>>> cfe-dev mailing list
>>> cfe-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180924/5cc05048/attachment.html>

On Mon, 24 Sep 2018, 20:40 Richard Trieu via cfe-dev, <
cfe-dev at lists.llvm.org> wrote:
> + Erik, who implemented DR1579
>
> Originally, I had the warning similar to GCC's warning, but took it out
> due to not having DR1579 implemented in clang (warning changed in r243594)
>
> Erik in r274291 implemented DR1579, although PR27785 didn't mention
> anything about std::move
>
> It looks like what's happening is that Clang and GCC handles the return
> differently.  Clang needs the std::move call to use the move constructor
> while GCC will use the move constructor with or without the std::move
> call.  This means that the warning is currently correct when running on
> either compiler.
>
> This is a reduced example.  Compiled with Clang, it will print "move
> constructor" then "copy constructor".  GCC will print
"move constructor"
> twice.
>
GCC gets the rule "wrong". The rule in question (
http://eel.is/c++draft/class.copy.elision#3.sentence-2) only applies when
the selected B constructor takes A&& as its parameter type.

#include <iostream>
> #include <memory>
>
> struct A {
>   A(int) {}
>   A() {  std::cout << "empty constructor"; }
>   A(const A&) {  std::cout << "copy constructor\n"; }
>   A(A&&) {  std::cout << "move constructor\n"; }
> };
> struct B {
>   B(A a) {}
> };
>
> A getA() { return A(1); }
>
> B run1() {
>   A a = getA();
>   return std::move(a);
> }
>
> B run2() {
>   A a = getA();
>   return a;
> }
>
> int main() {
>   std::cout << "with move:\n";
>   run1();
>   std::cout << "no move:\n";
>   run2();
> }
>
>
>
> On Tue, Sep 18, 2018 at 4:07 PM Richard Trieu <rtrieu at google.com>
wrote:
>
>> I'll look into the pessimizing move warning, then after that, the
>> deprecated copy warning.
>>
>> On Sun, Sep 16, 2018 at 10:41 AM Richard Smith <richard at
metafoo.co.uk>
>> wrote:
>>
>>> Yes, we should produce this warning in C++11 mode too. (I could be
>>> misrecalling, but I think the rationale for the current behaviour
is based
>>> on historical GCC behaviour.)
>>>
>>> On Sun, 16 Sep 2018, 10:04 David Blaikie via cfe-dev, <
>>> cfe-dev at lists.llvm.org> wrote:
>>>
>>>> Fair point made on that thread - that this is a DR, so
technically the
>>>> std::move is pessimizing even in C++11 mode. Richard: Any
thoughts on
>>>> generalizing the warning to cover these cases even in C++11
mode?
>>>>
>>>> On Sat, Sep 15, 2018 at 2:37 AM Dávid Bolvanský <
>>>> david.bolvansky at gmail.com> wrote:
>>>>
>>>>> There is a new discussion related to -Wredundant-move
warning on GCC
>>>>> bugzilla.
>>>>>
>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87300
>>>>>
>>>>> pi 14. 9. 2018 o 9:53 David Blaikie <dblaikie at
gmail.com> napísal(a):
>>>>>
>>>>>>
>>>>>>
>>>>>> On Fri, Sep 14, 2018 at 12:48 AM Stephan Bergmann <
>>>>>> sbergman at redhat.com> wrote:
>>>>>>
>>>>>>> On 13/09/2018 18:22, David Blaikie via llvm-dev
wrote:
>>>>>>> > On Thu, Sep 13, 2018 at 12:13 AM Dávid
Bolvanský via llvm-dev
>>>>>>> > <llvm-dev at lists.llvm.org
<mailto:llvm-dev at lists.llvm.org>> wrote:
>>>>>>> >
>>>>>>> 
/home/davidbolvansky/trunk/llvm/unittests/ExecutionEngine/Orc/CompileOnDemandLayerTest.cpp:79:40:
>>>>>>>
>>>>>>> >       required from here
>>>>>>> >
>>>>>>> 
/home/davidbolvansky/trunk/llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h:314:29:
>>>>>>> >     warning: redundant move in return
statement [-Wredundant-move]
>>>>>>> >     314 |         return std::move(Err);
>>>>>>>
>>>>>>> Note that the move (into the implicit
JITSymbol(Error) ctor) is only
>>>>>>> redundant if the compiler implements a fix for
>>>>>>>
<http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579>
>>>>>>> "Return by converting move constructor". 
(But not sure whether the
>>>>>>> LLVM
>>>>>>> compiler baselines imply that, anyway.
>>>>>>
>>>>>>
>>>>>> Looks like that was accepted for C++14, not 11. So I
don't believe
>>>>>> it's valid to remove the std::move in C++11 code.
(& I believe Clang's
>>>>>> warning correctly diagnoses this only in the right
language versions) - so
>>>>>> we probably want to disable the buggy GCC warning here,
then.
>>>>>>
>>>>>>
>>>>>>> In LibreOffice it forced me to
>>>>>>> introduce ugly #ifs, to not have to disable that
warning outright,
>>>>>>> <
>>>>>>>
https://cgit.freedesktop.org/libreoffice/core/commit/?id=dc06c8f4989fc28d0c31ebd333e53dfe0e0f5f66>
>>>>>>>
>>>>>>> "-Werror=redundant-move (GCC 9), take
two".)
>>>>>>>
>>>>>> _______________________________________________
>>>> cfe-dev mailing list
>>>> cfe-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>
>>> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180924/adfc9154/attachment.html>

On Tue, Sep 25, 2018 at 5:08 AM Richard Smith via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> On Mon, 24 Sep 2018, 20:40 Richard Trieu via cfe-dev, <
> cfe-dev at lists.llvm.org> wrote:
>
>> + Erik, who implemented DR1579
>>
>> Originally, I had the warning similar to GCC's warning, but took it
out
>> due to not having DR1579 implemented in clang (warning changed in
r243594)
>>
>> Erik in r274291 implemented DR1579, although PR27785 didn't mention
>> anything about std::move
>>
>> It looks like what's happening is that Clang and GCC handles the
return
>> differently.  Clang needs the std::move call to use the move
constructor
>> while GCC will use the move constructor with or without the std::move
>> call.  This means that the warning is currently correct when running on
>> either compiler.
>>
>> This is a reduced example.  Compiled with Clang, it will print
"move
>> constructor" then "copy constructor".  GCC will print
"move constructor"
>> twice.
>>
>
> GCC gets the rule "wrong". The rule in question (
> http://eel.is/c++draft/class.copy.elision#3.sentence-2) only applies when
> the selected B constructor takes A&& as its parameter type.
>
> But the rule gets the rule wrong too -- EWG wants that condition removed,
so that it's never necessary to write
  return std::move(local_var);

I think the rule was written as it is to avoid breaking (obscure) working
code, but before we had sufficient experience with rvalue references to
understand how reasonable it is to pass by value in such cases.  The time
has come to simplify it.

I have a patch in progress, but lacked the time to finish updating test
cases etc.  (And I'm on vacation right now.)

-- James
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180926/48ea4e62/attachment.html>

On 25/09/2018 05:40, Richard Trieu via cfe-dev wrote:
> It looks like what's happening is that Clang and GCC handles the return
> differently.  Clang needs the std::move call to use the move constructor 
> while GCC will use the move constructor with or without the std::move 
> call.  This means that the warning is currently correct when running on 
> either compiler.
> 
> This is a reduced example.  Compiled with Clang, it will print "move 
> constructor" then "copy constructor".  GCC will print
"move constructor"
> twice.
For the erroneous GCC behavior, see 
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87150> "move ctor
wrongly
chosen in return stmt (derived vs. base)".