Archived issue #0030683
Coding Rules - eliminate CLang compiler warnings -Wreturn-std-move
Description
CLang (XCode 10.2) generates the following compiler warnings in a numerous (100+) places:
The usual location for this warning in OCCT is a function returning TopoDS_Shape from a local variable declared as TopoDS_Compound.
Note that motivation around -Wreturn-std-move in CLang seems to be unrelated to warning appearance in OCCT - hence might be a compiler bug.
Although the rationale can be found - indeed, all sub-classes of TopoDS_Shape a dummy and can be used for moving to TopoDS_Shape without copy.
It might be reasonable also marking TopoDS_Shape sub-classes as final classes.
Apple LLVM version 10.0.1 (clang-1001.0.46.3)
Target: x86_64-apple-darwin18.5.0
...
[user path removed]/Develop/occt.git/src/BRepTools/BRepTools_Quilt.cxx:527: warning: local variable 'result' will be copied despite being returned by name [-Wreturn-std-move]
return result;
^~~~~~
The usual location for this warning in OCCT is a function returning TopoDS_Shape from a local variable declared as TopoDS_Compound.
Note that motivation around -Wreturn-std-move in CLang seems to be unrelated to warning appearance in OCCT - hence might be a compiler bug.
Although the rationale can be found - indeed, all sub-classes of TopoDS_Shape a dummy and can be used for moving to TopoDS_Shape without copy.
It might be reasonable also marking TopoDS_Shape sub-classes as final classes.
Public activity
24 archived notes
Participants are labeled by their role within this record.
Branch [archived branch] has been created by Author.
[revision removed]
Detailed log of new commits:
Author: Author
Date: Mon Apr 29 19:51:34 2019 +0300
0030683: Coding Rules - eliminate CLang compiler warnings -Wreturn-std-move
Declare template move TopoDS_Shape constructor for sub-classes within fake hierarchy.
[revision removed]
Detailed log of new commits:
Author: Author
Date: Mon Apr 29 19:51:34 2019 +0300
0030683: Coding Rules - eliminate CLang compiler warnings -Wreturn-std-move
Declare template move TopoDS_Shape constructor for sub-classes within fake hierarchy.
Branch [archived branch] has been updated forcibly by Author.
[revision removed]
[revision removed]
Patch is ready for review.
http://jenkins-test-12.nnov.opencascade.com:8080/view/CR30683-master-KGV/
http://jenkins-test-12.nnov.opencascade.com:8080/view/CR30683-master-KGV/
Branch [archived branch] has been updated forcibly by Author.
[revision removed]
[revision removed]
Looks like there are some problems with your code changes:
std::enable_if → opencascade::std::enable_if
The type of myLocation member variable is TopLoc_Location. It doesn’t have neither copy nor move constructor (only implicit conversion constructor from an object of gp_Trsf type and implicit conversion operator to gp_Trsf). So the explicit move in this place looks strange.
const TopoDS_Shape&& → TopoDS_Shape&&
It is more safe to use default template parameters instead of function default template parameters in such situations
...to prevent the possibility of writing the code like
But note that default template parameters for function templates were introduced only in C++11 and might not be supported in old versions of Visual Studio.
The comment doesn't match the code: this constructor isn't generalized move constructor in fact because it may silently take lvalue too, not only rvalue. T2&& isn't an rvalue reference here, it is a forwarding reference (previously widely known as "universal reference").
TopoDS_Shape (T2&& theOther, typename std::enable_if<opencascade::is_base_but_not_same<TopoDS_Shape, T2>::value>::type* = nullptr)
std::enable_if → opencascade::std::enable_if
myLocation(std::move (theOther.myLocation))
The type of myLocation member variable is TopLoc_Location. It doesn’t have neither copy nor move constructor (only implicit conversion constructor from an object of gp_Trsf type and implicit conversion operator to gp_Trsf). So the explicit move in this place looks strange.
TopoDS_Shape& operator= (const TopoDS_Shape&& theOther)
const TopoDS_Shape&& → TopoDS_Shape&&
template <class T2> TopoDS_Shape (T2&& theOther, typename std::enable_if<opencascade::is_base_but_not_same<TopoDS_Shape, T2>::value>::type* = nullptr)
It is more safe to use default template parameters instead of function default template parameters in such situations
template <class T2, typename opencascade::std::enable_if<opencascade::is_base_but_not_same<TopoDS_Shape, T2>::value>::type* = nullptr> TopoDS_Shape (T2&& theOther)
...to prevent the possibility of writing the code like
TopoDS_Compound aCompound; TopoDS_Shape aShape(std::move(aCompound), nullptr);
But note that default template parameters for function templates were introduced only in C++11 and might not be supported in old versions of Visual Studio.
//! Generalized move constructor for sub-classes (TopoDS_Shape hierarchy declares only fake sub-classes with no extra fields). template <class T2> TopoDS_Shape (T2&& theOther, typename std::enable_if<opencascade::is_base_but_not_same<TopoDS_Shape, T2>::value>::type* = nullptr)
The comment doesn't match the code: this constructor isn't generalized move constructor in fact because it may silently take lvalue too, not only rvalue. T2&& isn't an rvalue reference here, it is a forwarding reference (previously widely known as "universal reference").
Branch [archived branch] has been updated forcibly by Author.
[revision removed]
[revision removed]
> T2&& isn't an rvalue reference here, it is a forwarding reference
>(previously widely known as "universal reference").
Corrected to use std::forward instead of std::move.
>(previously widely known as "universal reference").
Corrected to use std::forward instead of std::move.
Branch [archived branch] has been updated by Participant.
[revision removed]
Detailed log of new commits:
Author: abv
Date: Sun May 5 15:17:50 2019 +0300
Added move constructor and operator for TopLoc_SListOfItemLocation, and generalized move operator for TopoDS_Shape.
Macro OCCT_NO_RVALUE_REFERENCE is used in Standard_Handle.hxx instead of direct check of compiler version.
[revision removed]
Detailed log of new commits:
Author: abv
Date: Sun May 5 15:17:50 2019 +0300
Added move constructor and operator for TopLoc_SListOfItemLocation, and generalized move operator for TopoDS_Shape.
Macro OCCT_NO_RVALUE_REFERENCE is used in Standard_Handle.hxx instead of direct check of compiler version.
I have pushed some corrections in the same branch, please look:
1. Added move constructor and assignment operator to class TopLoc_SListOfItemLocation - this shall make move constructor and assignment of TopLoc_Location class (which should be generated automatically by compiler) efficient
2. In TopoDS_Shape.hxx, only universal constructor and assignment operator are kept - they should work for the class itself and sub-classes, in both move and copy use cases
3. In Standard_Handle.hxx, macro OCCT_NO_RVALUE_REFERENCE is used in relevant places instead of direct check of compiler version
Regarding above comments by Timur:
(a) Use of the same pattern with std::forward for copying / moving all class fields is reasonable even if particular field has no move constructor / assignment, just for consistency
(b) Replacement std::enable_if → opencascade::std::enable_if does not seem to be really needed since this code is within #ifdefs restricting it to compilers supporting perfect forwarding, and we shall assume that all these compilers also provide std::enable_if out of the box
(c) Use of default parameter of constructor instead of default template argument is justified by the need to support VC++ 11 which does not support default template arguments for function templates (according to https://docs.microsoft.com/en-us/previous-versions/hh567368(v=vs.140) )
(d) On use of term "generalized": I could not find what term is used nowadays for this kind of functions, then used term "universal" proposed by Scott Mayer years ago
----
I have checked whether these changes reduce number of copy operations on shapes (as they should), and was able to detect up to 6% decrease of number of increments of counter of Standard_Transient during execution of DRAW script samples/tcl/bottle.tcl (17.9 M increments vs. 19.2 M on master).
Notable the number of increments of the counter is not the same from run to run after execution of almost any non-trivial command, but this is separate issue...
I have checked with VS 2017 only, thus did not check that my version compiles cleanly with CLang.
1. Added move constructor and assignment operator to class TopLoc_SListOfItemLocation - this shall make move constructor and assignment of TopLoc_Location class (which should be generated automatically by compiler) efficient
2. In TopoDS_Shape.hxx, only universal constructor and assignment operator are kept - they should work for the class itself and sub-classes, in both move and copy use cases
3. In Standard_Handle.hxx, macro OCCT_NO_RVALUE_REFERENCE is used in relevant places instead of direct check of compiler version
Regarding above comments by Timur:
(a) Use of the same pattern with std::forward for copying / moving all class fields is reasonable even if particular field has no move constructor / assignment, just for consistency
(b) Replacement std::enable_if → opencascade::std::enable_if does not seem to be really needed since this code is within #ifdefs restricting it to compilers supporting perfect forwarding, and we shall assume that all these compilers also provide std::enable_if out of the box
(c) Use of default parameter of constructor instead of default template argument is justified by the need to support VC++ 11 which does not support default template arguments for function templates (according to https://docs.microsoft.com/en-us/previous-versions/hh567368(v=vs.140) )
(d) On use of term "generalized": I could not find what term is used nowadays for this kind of functions, then used term "universal" proposed by Scott Mayer years ago
----
I have checked whether these changes reduce number of copy operations on shapes (as they should), and was able to detect up to 6% decrease of number of increments of counter of Standard_Transient during execution of DRAW script samples/tcl/bottle.tcl (17.9 M increments vs. 19.2 M on master).
Notable the number of increments of the counter is not the same from run to run after execution of almost any non-trivial command, but this is separate issue...
I have checked with VS 2017 only, thus did not check that my version compiles cleanly with CLang.
Branch [archived branch] has been created by Author.
[revision removed]
Detailed log of new commits:
Author: Author
Date: Mon Apr 29 19:51:34 2019 +0300
0030683: Coding Rules - eliminate CLang compiler warnings -Wreturn-std-move
Added generalized move constructor and operator for sub-classes of TopoDS_Shape.
Added move constructor and operator for TopLoc_SListOfItemLocation.
Macro OCCT_NO_RVALUE_REFERENCE is used in Standard_Handle.hxx instead of direct check of compiler version.
[revision removed]
Detailed log of new commits:
Author: Author
Date: Mon Apr 29 19:51:34 2019 +0300
0030683: Coding Rules - eliminate CLang compiler warnings -Wreturn-std-move
Added generalized move constructor and operator for sub-classes of TopoDS_Shape.
Added move constructor and operator for TopLoc_SListOfItemLocation.
Macro OCCT_NO_RVALUE_REFERENCE is used in Standard_Handle.hxx instead of direct check of compiler version.
Looks like such generalized constructor is called only in the case of object moving, not copying (tested on clang 8.0.0 with flags -std=c++11 -Wall -Wextra -Werror -pedantic-errors): https://wandbox.org/permlink/QhuPCDQ0ibHm3FNJ
Please take the patch.
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR30683-master-KGV/
> Looks like such generalized constructor is called only
> in the case of object moving, not copying
Since generated copy constructor should do the same thing as our template, this shouldn't be the issue (apart from extra confusion).
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR30683-master-KGV/
> Looks like such generalized constructor is called only
> in the case of object moving, not copying
Since generated copy constructor should do the same thing as our template, this shouldn't be the issue (apart from extra confusion).
>> Looks like such generalized constructor is called only in the case of object moving, not copying
This is so, because in case of lvalue passed to such generalized constructor T2 is deduced as a reference (lvalue reference to be more precise) to the type of the passed object. The possible solution is to use std::remove_reference: https://wandbox.org/permlink/mAEj6yUWVIfIzauT
This is so, because in case of lvalue passed to such generalized constructor T2 is deduced as a reference (lvalue reference to be more precise) to the type of the passed object. The possible solution is to use std::remove_reference: https://wandbox.org/permlink/mAEj6yUWVIfIzauT
Branch [archived branch] has been created by Participant.
[revision removed]
Detailed log of new commits:
Author: Author
Date: Mon Apr 29 19:51:34 2019 +0300
0030683: Coding Rules - eliminate CLang compiler warnings -Wreturn-std-move
Added generalized move constructor and assignment operator to initialize TopoDS_Shape by object of this or derived type.
Added move constructor and assignment operator for TopLoc_SListOfItemLocation.
Macro OCCT_NO_RVALUE_REFERENCE is used in Standard_Handle.hxx instead of direct check of compiler version.
[revision removed]
Detailed log of new commits:
Author: Author
Date: Mon Apr 29 19:51:34 2019 +0300
0030683: Coding Rules - eliminate CLang compiler warnings -Wreturn-std-move
Added generalized move constructor and assignment operator to initialize TopoDS_Shape by object of this or derived type.
Added move constructor and assignment operator for TopLoc_SListOfItemLocation.
Macro OCCT_NO_RVALUE_REFERENCE is used in Standard_Handle.hxx instead of direct check of compiler version.
Please take branch CR30683_2 for integration: it is the same as CR30683_1 except that some comments are corrected
Combination -
OCCT branch : [archived branch]
master SHA - [revision removed]
[revision removed]
Products branch : [archived branch] SHA - [revision removed]
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.
Number of compiler warnings:
No new/fixed warnings
Regressions/Differences/Improvements:
No regressions/differences
CPU differences:
Debian80-64:
OCCT
Total CPU difference: 16267.400000000021 / 16286.290000000015 [-0.12%]
Products
Total CPU difference: 10492.690000000055 / 10492.700000000066 [-0.00%]
Windows-64-VC14:
OCCT
Total CPU difference: 17652.234375 / 17645.203125 [+0.04%]
Products
Total CPU difference: 12093.125 / 12107.4375 [-0.12%]
Image differences :
No differences that require special attention
Memory differences :
No differences that require special attention
OCCT branch : [archived branch]
master SHA - [revision removed]
[revision removed]
Products branch : [archived branch] SHA - [revision removed]
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.
Number of compiler warnings:
No new/fixed warnings
Regressions/Differences/Improvements:
No regressions/differences
CPU differences:
Debian80-64:
OCCT
Total CPU difference: 16267.400000000021 / 16286.290000000015 [-0.12%]
Products
Total CPU difference: 10492.690000000055 / 10492.700000000066 [-0.00%]
Windows-64-VC14:
OCCT
Total CPU difference: 17652.234375 / 17645.203125 [+0.04%]
Products
Total CPU difference: 12093.125 / 12107.4375 [-0.12%]
Image differences :
No differences that require special attention
Memory differences :
No differences that require special attention
CentOS64-64:
GCC feature list history:
> The CXX compiler identification is GNU 4.4.7
GCC feature list history:
> Rvalue references: N2118 GCC 4.3 > Null pointer constant:N2431 GCC 4.6
Branch [archived branch] has been updated forcibly by Author.
[revision removed]
[revision removed]
Please check compilation of updated patch on mentioned platform.
Branch [archived branch] has been deleted by Participant.
[revision removed]
[revision removed]
Branch [archived branch] has been deleted by Participant.
[revision removed]
[revision removed]
Branch [archived branch] has been deleted by Participant.
[revision removed]
[revision removed]
Related records