Re: [llvm-dev] New warnings when building trunk with GCC 9

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] New warnings when building trunk with GCC 9

via cfe-dev
Great to see GCC catching the deprecated ctor case - would be great to get that implemented in Clang at some point (& enabled for LLVM builds). (rtrieu@)
The vector one looks like a false positive? You're allowed to form a pointer to an element one past the end of a singular object but it looks like that's what GCC's warning on.
Initializer list - maybe a false positive too, given ArrayRef is intended to refer to a temporary object - I think the initializer_list's lifetime is to teh end of the full expression where it was written, so this can still be OK.
The ORC/redundant-move one looks correct - I wonder why Clang didn't diagnose that (rtrieu@)?

On Thu, Sep 13, 2018 at 12:13 AM Dávid Bolvanský via llvm-dev <[hidden email]> wrote:
Hello,

GCC 9.0 introduces a new warning checkers and some of them found possible issues in LLVM.

In file included from /home/davidbolvansky/trunk/llvm/include/llvm/Analysis/LazyCallGraph.h:38,
                 from /home/davidbolvansky/trunk/llvm/unittests/Analysis/LazyCallGraphTest.cpp:10:
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/ArrayRef.h: In instantiation of ‘llvm::ArrayRef<T>::ArrayRef(const std::initializer_list<_Tp>&) [with T = llvm::LazyCallGraph::Node*]’:
/home/davidbolvansky/trunk/llvm/unittests/Analysis/LazyCallGraphTest.cpp:1169:52:   required from here
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/ArrayRef.h:102:37: warning: initializing ‘llvm::ArrayRef<llvm::LazyCallGraph::Node*>::Data’ from ‘std::initializer_list<llvm::LazyCallGraph::Node*>::begin’ does not extend the lifetime of the underlying array [-Winit-list-lifetime]

In file included from /home/davidbolvansky/trunk/llvm/unittests/ADT/SmallVectorTest.cpp:14:
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/SmallVector.h: In member function ‘virtual void {anonymous}::SmallVectorTest_InitializerList_Test::TestBody()’:
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/SmallVector.h:502:7: warning: array subscript 1 is outside array bounds of ‘int [1]’ [-Warray-bounds]
502 |       ++EltPtr;
/home/davidbolvansky/trunk/llvm/unittests/ADT/SmallVectorTest.cpp:994:30: note: while referencing ‘<anonymous>’
994 |   V2.insert(V2.begin() + 1, 5);

/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);

In file included from /home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-spec-builders.h:75,
                 from /home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h:43,
                 from /home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock.h:61,
                 from /home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/Error.h:17,
                 from /home/davidbolvansky/trunk/llvm/unittests/DebugInfo/MSF/MappedBlockStreamTest.cpp:15:
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-matchers.h: In instantiation of ‘testing::internal::PredicateFormatterFromMatcher<M>::PredicateFormatterFromMatcher(M) [with M = llvm::FailedMatcher]’:
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-matchers.h:1880:10:   required from ‘testing::internal::PredicateFormatterFromMatcher<M> testing::internal::MakePredicateFormatterFromMatcher(M) [with M = llvm::FailedMatcher]’
/home/davidbolvansky/trunk/llvm/unittests/DebugInfo/MSF/MappedBlockStreamTest.cpp:96:3:   required from here
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-matchers.h:1836:75: warning: implicitly-declared ‘constexpr llvm::FailedMatcher::FailedMatcher(const llvm::FailedMatcher&)’ is deprecated [-Wdeprecated-copy]
1836 |   explicit PredicateFormatterFromMatcher(M m) : matcher_(internal::move(m)) {}
     |                                                                           ^
In file included from /home/davidbolvansky/trunk/llvm/utils/unittest/googletest/include/gtest/gtest-printers.h:103,
                 from /home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/SupportHelpers.h:16,
                 from /home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/Error.h:15,
                 from /home/davidbolvansky/trunk/llvm/unittests/DebugInfo/MSF/MappedBlockStreamTest.cpp:15:
/home/davidbolvansky/trunk/llvm/utils/unittest/googletest/include/gtest/internal/gtest-port.h:873:8: note: because ‘llvm::FailedMatcher’ has user-provided ‘void llvm::FailedMatcher::operator=(const llvm::FailedMatcher&)’
873 |   void operator=(type const &)
    |        ^~~~~~~~
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-matchers.h:1417:5: note: in expansion of macro ‘GTEST_DISALLOW_ASSIGN_’
1417 |     GTEST_DISALLOW_ASSIGN_(name##Matcher);\
     |     ^~~~~~~~~~~~~~~~~~~~~~
/home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/Error.h:145:1: note: in expansion of macro ‘MATCHER’
145 | MATCHER(Failed, "") { return !arg.Success(); }






_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] New warnings when building trunk with GCC 9

via cfe-dev
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
> <[hidden email] <mailto:[hidden email]>> 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.  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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] New warnings when building trunk with GCC 9

via cfe-dev


On Fri, Sep 14, 2018 at 12:48 AM Stephan Bergmann <[hidden email]> 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
> <[hidden email] <mailto:[hidden email]>> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] New warnings when building trunk with GCC 9

via cfe-dev
There is a new discussion related to -Wredundant-move warning on GCC bugzilla.


pi 14. 9. 2018 o 9:53 David Blaikie <[hidden email]> napísal(a):


On Fri, Sep 14, 2018 at 12:48 AM Stephan Bergmann <[hidden email]> 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
> <[hidden email] <mailto:[hidden email]>> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] New warnings when building trunk with GCC 9

via cfe-dev
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ý <[hidden email]> wrote:
There is a new discussion related to -Wredundant-move warning on GCC bugzilla.


pi 14. 9. 2018 o 9:53 David Blaikie <[hidden email]> napísal(a):


On Fri, Sep 14, 2018 at 12:48 AM Stephan Bergmann <[hidden email]> 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
> <[hidden email] <mailto:[hidden email]>> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] New warnings when building trunk with GCC 9

via cfe-dev
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, <[hidden email]> 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ý <[hidden email]> wrote:
There is a new discussion related to -Wredundant-move warning on GCC bugzilla.


pi 14. 9. 2018 o 9:53 David Blaikie <[hidden email]> napísal(a):


On Fri, Sep 14, 2018 at 12:48 AM Stephan Bergmann <[hidden email]> 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
> <[hidden email] <mailto:[hidden email]>> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] New warnings when building trunk with GCC 9

via cfe-dev
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 <[hidden email]> 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, <[hidden email]> 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ý <[hidden email]> wrote:
There is a new discussion related to -Wredundant-move warning on GCC bugzilla.


pi 14. 9. 2018 o 9:53 David Blaikie <[hidden email]> napísal(a):


On Fri, Sep 14, 2018 at 12:48 AM Stephan Bergmann <[hidden email]> 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
> <[hidden email] <mailto:[hidden email]>> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] New warnings when building trunk with GCC 9

via cfe-dev
+ 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 <[hidden email]> 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 <[hidden email]> 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, <[hidden email]> 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ý <[hidden email]> wrote:
There is a new discussion related to -Wredundant-move warning on GCC bugzilla.


pi 14. 9. 2018 o 9:53 David Blaikie <[hidden email]> napísal(a):


On Fri, Sep 14, 2018 at 12:48 AM Stephan Bergmann <[hidden email]> 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
> <[hidden email] <mailto:[hidden email]>> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] New warnings when building trunk with GCC 9

via cfe-dev
On Mon, 24 Sep 2018, 20:40 Richard Trieu via cfe-dev, <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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, <[hidden email]> 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ý <[hidden email]> wrote:
There is a new discussion related to -Wredundant-move warning on GCC bugzilla.


pi 14. 9. 2018 o 9:53 David Blaikie <[hidden email]> napísal(a):


On Fri, Sep 14, 2018 at 12:48 AM Stephan Bergmann <[hidden email]> 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
> <[hidden email] <mailto:[hidden email]>> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] New warnings when building trunk with GCC 9

via cfe-dev
On Tue, Sep 25, 2018 at 5:08 AM Richard Smith via cfe-dev <[hidden email]> wrote:
On Mon, 24 Sep 2018, 20:40 Richard Trieu via cfe-dev, <[hidden email]> 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 

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] New warnings when building trunk with GCC 9

via cfe-dev
In reply to this post by via cfe-dev
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)".
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] New warnings when building trunk with GCC 9

via cfe-dev
In reply to this post by via cfe-dev
So, Clang does have a warning for deprecated copy, except that it's buried in -Wdeprecated without its own flag.  There's a few other differences too.  GCC will warn once per call to the deprecated function while Clang will warn once per deprecated function.  GCC will not consider functions that are "= default" or "= delete" while Clang will.  That means for "struct S { ~S() = default; };" using S's implicit copy constructor will cause Clang to warn while GCC does not.  GCC uses "user-provided" while Clang says "user-declared"

$ cat test.cc
struct S {
  ~S();
};

S s = s;

$ gcc test.cc -Wdeprecated-copy
test.cc:5:7: warning: implicitly-declared 'constexpr S::S(const S&)' is deprecated [-Wdeprecated-copy]
5 | S s = s;
  |       ^
test.cc:2:3: note: because 'S' has user-provided 'S::~S()'
2 |   ~S();
  |   ^

$ clang test.cc -Wdeprecated
test:2:3: warning: definition of implicit copy constructor for 'S' is deprecated because it has a user-declared destructor [-Wdeprecated]
  ~S();
  ^
test:5:7: note: in implicit copy constructor for 'S' first required here
S s = s;
      ^

This is a patch to give Clang a -Wdeprecated-copy, which also ignores explicitly defaulted/deleted functions like GCC.

On Thu, Sep 13, 2018 at 9:22 AM David Blaikie <[hidden email]> wrote:
Great to see GCC catching the deprecated ctor case - would be great to get that implemented in Clang at some point (& enabled for LLVM builds). (rtrieu@)
The vector one looks like a false positive? You're allowed to form a pointer to an element one past the end of a singular object but it looks like that's what GCC's warning on.
Initializer list - maybe a false positive too, given ArrayRef is intended to refer to a temporary object - I think the initializer_list's lifetime is to teh end of the full expression where it was written, so this can still be OK.
The ORC/redundant-move one looks correct - I wonder why Clang didn't diagnose that (rtrieu@)?

On Thu, Sep 13, 2018 at 12:13 AM Dávid Bolvanský via llvm-dev <[hidden email]> wrote:
Hello,

GCC 9.0 introduces a new warning checkers and some of them found possible issues in LLVM.

In file included from /home/davidbolvansky/trunk/llvm/include/llvm/Analysis/LazyCallGraph.h:38,
                 from /home/davidbolvansky/trunk/llvm/unittests/Analysis/LazyCallGraphTest.cpp:10:
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/ArrayRef.h: In instantiation of ‘llvm::ArrayRef<T>::ArrayRef(const std::initializer_list<_Tp>&) [with T = llvm::LazyCallGraph::Node*]’:
/home/davidbolvansky/trunk/llvm/unittests/Analysis/LazyCallGraphTest.cpp:1169:52:   required from here
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/ArrayRef.h:102:37: warning: initializing ‘llvm::ArrayRef<llvm::LazyCallGraph::Node*>::Data’ from ‘std::initializer_list<llvm::LazyCallGraph::Node*>::begin’ does not extend the lifetime of the underlying array [-Winit-list-lifetime]

In file included from /home/davidbolvansky/trunk/llvm/unittests/ADT/SmallVectorTest.cpp:14:
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/SmallVector.h: In member function ‘virtual void {anonymous}::SmallVectorTest_InitializerList_Test::TestBody()’:
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/SmallVector.h:502:7: warning: array subscript 1 is outside array bounds of ‘int [1]’ [-Warray-bounds]
502 |       ++EltPtr;
/home/davidbolvansky/trunk/llvm/unittests/ADT/SmallVectorTest.cpp:994:30: note: while referencing ‘<anonymous>’
994 |   V2.insert(V2.begin() + 1, 5);

/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);

In file included from /home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-spec-builders.h:75,
                 from /home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h:43,
                 from /home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock.h:61,
                 from /home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/Error.h:17,
                 from /home/davidbolvansky/trunk/llvm/unittests/DebugInfo/MSF/MappedBlockStreamTest.cpp:15:
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-matchers.h: In instantiation of ‘testing::internal::PredicateFormatterFromMatcher<M>::PredicateFormatterFromMatcher(M) [with M = llvm::FailedMatcher]’:
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-matchers.h:1880:10:   required from ‘testing::internal::PredicateFormatterFromMatcher<M> testing::internal::MakePredicateFormatterFromMatcher(M) [with M = llvm::FailedMatcher]’
/home/davidbolvansky/trunk/llvm/unittests/DebugInfo/MSF/MappedBlockStreamTest.cpp:96:3:   required from here
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-matchers.h:1836:75: warning: implicitly-declared ‘constexpr llvm::FailedMatcher::FailedMatcher(const llvm::FailedMatcher&)’ is deprecated [-Wdeprecated-copy]
1836 |   explicit PredicateFormatterFromMatcher(M m) : matcher_(internal::move(m)) {}
     |                                                                           ^
In file included from /home/davidbolvansky/trunk/llvm/utils/unittest/googletest/include/gtest/gtest-printers.h:103,
                 from /home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/SupportHelpers.h:16,
                 from /home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/Error.h:15,
                 from /home/davidbolvansky/trunk/llvm/unittests/DebugInfo/MSF/MappedBlockStreamTest.cpp:15:
/home/davidbolvansky/trunk/llvm/utils/unittest/googletest/include/gtest/internal/gtest-port.h:873:8: note: because ‘llvm::FailedMatcher’ has user-provided ‘void llvm::FailedMatcher::operator=(const llvm::FailedMatcher&)’
873 |   void operator=(type const &)
    |        ^~~~~~~~
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-matchers.h:1417:5: note: in expansion of macro ‘GTEST_DISALLOW_ASSIGN_’
1417 |     GTEST_DISALLOW_ASSIGN_(name##Matcher);\
     |     ^~~~~~~~~~~~~~~~~~~~~~
/home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/Error.h:145:1: note: in expansion of macro ‘MATCHER’
145 | MATCHER(Failed, "") { return !arg.Success(); }






_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] New warnings when building trunk with GCC 9

via cfe-dev

These are the needed cleanup for the new -Wdeprecated-copy warning.  LatticeCell seems to be the only interesting case, with logic in the copy assignment operator not being used in the copy constructor.  The rest are pretty straight forward.

On Wed, Oct 3, 2018 at 7:07 PM Richard Trieu <[hidden email]> wrote:
So, Clang does have a warning for deprecated copy, except that it's buried in -Wdeprecated without its own flag.  There's a few other differences too.  GCC will warn once per call to the deprecated function while Clang will warn once per deprecated function.  GCC will not consider functions that are "= default" or "= delete" while Clang will.  That means for "struct S { ~S() = default; };" using S's implicit copy constructor will cause Clang to warn while GCC does not.  GCC uses "user-provided" while Clang says "user-declared"

$ cat test.cc
struct S {
  ~S();
};

S s = s;

$ gcc test.cc -Wdeprecated-copy
test.cc:5:7: warning: implicitly-declared 'constexpr S::S(const S&)' is deprecated [-Wdeprecated-copy]
5 | S s = s;
  |       ^
test.cc:2:3: note: because 'S' has user-provided 'S::~S()'
2 |   ~S();
  |   ^

$ clang test.cc -Wdeprecated
test:2:3: warning: definition of implicit copy constructor for 'S' is deprecated because it has a user-declared destructor [-Wdeprecated]
  ~S();
  ^
test:5:7: note: in implicit copy constructor for 'S' first required here
S s = s;
      ^

This is a patch to give Clang a -Wdeprecated-copy, which also ignores explicitly defaulted/deleted functions like GCC.

On Thu, Sep 13, 2018 at 9:22 AM David Blaikie <[hidden email]> wrote:
Great to see GCC catching the deprecated ctor case - would be great to get that implemented in Clang at some point (& enabled for LLVM builds). (rtrieu@)
The vector one looks like a false positive? You're allowed to form a pointer to an element one past the end of a singular object but it looks like that's what GCC's warning on.
Initializer list - maybe a false positive too, given ArrayRef is intended to refer to a temporary object - I think the initializer_list's lifetime is to teh end of the full expression where it was written, so this can still be OK.
The ORC/redundant-move one looks correct - I wonder why Clang didn't diagnose that (rtrieu@)?

On Thu, Sep 13, 2018 at 12:13 AM Dávid Bolvanský via llvm-dev <[hidden email]> wrote:
Hello,

GCC 9.0 introduces a new warning checkers and some of them found possible issues in LLVM.

In file included from /home/davidbolvansky/trunk/llvm/include/llvm/Analysis/LazyCallGraph.h:38,
                 from /home/davidbolvansky/trunk/llvm/unittests/Analysis/LazyCallGraphTest.cpp:10:
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/ArrayRef.h: In instantiation of ‘llvm::ArrayRef<T>::ArrayRef(const std::initializer_list<_Tp>&) [with T = llvm::LazyCallGraph::Node*]’:
/home/davidbolvansky/trunk/llvm/unittests/Analysis/LazyCallGraphTest.cpp:1169:52:   required from here
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/ArrayRef.h:102:37: warning: initializing ‘llvm::ArrayRef<llvm::LazyCallGraph::Node*>::Data’ from ‘std::initializer_list<llvm::LazyCallGraph::Node*>::begin’ does not extend the lifetime of the underlying array [-Winit-list-lifetime]

In file included from /home/davidbolvansky/trunk/llvm/unittests/ADT/SmallVectorTest.cpp:14:
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/SmallVector.h: In member function ‘virtual void {anonymous}::SmallVectorTest_InitializerList_Test::TestBody()’:
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/SmallVector.h:502:7: warning: array subscript 1 is outside array bounds of ‘int [1]’ [-Warray-bounds]
502 |       ++EltPtr;
/home/davidbolvansky/trunk/llvm/unittests/ADT/SmallVectorTest.cpp:994:30: note: while referencing ‘<anonymous>’
994 |   V2.insert(V2.begin() + 1, 5);

/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);

In file included from /home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-spec-builders.h:75,
                 from /home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h:43,
                 from /home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock.h:61,
                 from /home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/Error.h:17,
                 from /home/davidbolvansky/trunk/llvm/unittests/DebugInfo/MSF/MappedBlockStreamTest.cpp:15:
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-matchers.h: In instantiation of ‘testing::internal::PredicateFormatterFromMatcher<M>::PredicateFormatterFromMatcher(M) [with M = llvm::FailedMatcher]’:
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-matchers.h:1880:10:   required from ‘testing::internal::PredicateFormatterFromMatcher<M> testing::internal::MakePredicateFormatterFromMatcher(M) [with M = llvm::FailedMatcher]’
/home/davidbolvansky/trunk/llvm/unittests/DebugInfo/MSF/MappedBlockStreamTest.cpp:96:3:   required from here
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-matchers.h:1836:75: warning: implicitly-declared ‘constexpr llvm::FailedMatcher::FailedMatcher(const llvm::FailedMatcher&)’ is deprecated [-Wdeprecated-copy]
1836 |   explicit PredicateFormatterFromMatcher(M m) : matcher_(internal::move(m)) {}
     |                                                                           ^
In file included from /home/davidbolvansky/trunk/llvm/utils/unittest/googletest/include/gtest/gtest-printers.h:103,
                 from /home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/SupportHelpers.h:16,
                 from /home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/Error.h:15,
                 from /home/davidbolvansky/trunk/llvm/unittests/DebugInfo/MSF/MappedBlockStreamTest.cpp:15:
/home/davidbolvansky/trunk/llvm/utils/unittest/googletest/include/gtest/internal/gtest-port.h:873:8: note: because ‘llvm::FailedMatcher’ has user-provided ‘void llvm::FailedMatcher::operator=(const llvm::FailedMatcher&)’
873 |   void operator=(type const &)
    |        ^~~~~~~~
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-matchers.h:1417:5: note: in expansion of macro ‘GTEST_DISALLOW_ASSIGN_’
1417 |     GTEST_DISALLOW_ASSIGN_(name##Matcher);\
     |     ^~~~~~~~~~~~~~~~~~~~~~
/home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/Error.h:145:1: note: in expansion of macro ‘MATCHER’
145 | MATCHER(Failed, "") { return !arg.Success(); }






_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev