MantisBT - Open CASCADE
View Issue Details
0030683Open CASCADE[OCCT] OCCT:Codingpublic2019-04-29 19:172019-05-08 13:41
kgv 
bugmaster 
normalminor 
verifiedfixed 
MacOSX
[OCCT] 7.3.0 
[OCCT] 7.4.0 
Not required
0030683: Coding Rules - eliminate CLang compiler warnings -Wreturn-std-move
CLang (XCode 10.2) generates the following compiler warnings in a numerous (100+) places:
Apple LLVM version 10.0.1 (clang-1001.0.46.3)
Target: x86_64-apple-darwin18.5.0
...
/Users/kgv/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.
No tags attached.
related to 0030681verified bugmaster Configuration - suppress OpenGL deprecation warning on macOS 10.14 
Issue History
2019-04-29 19:17kgvNew Issue
2019-04-29 19:17kgvAssigned To => kgv
2019-04-29 19:17kgvDescription Updatedbug_revision_view_page.php?rev_id=21107#r21107
2019-04-29 19:17kgvRelationship addedrelated to 0030681
2019-04-29 19:58gitNote Added: 0084054
2019-04-29 22:48gitNote Added: 0084061
2019-04-30 08:08kgvNote Added: 0084063
2019-04-30 08:08kgvAssigned Tokgv => abv
2019-04-30 08:08kgvStatusnew => resolved
2019-04-30 13:10kgvDescription Updatedbug_revision_view_page.php?rev_id=21117#r21117
2019-04-30 13:10kgvDescription Updatedbug_revision_view_page.php?rev_id=21118#r21118
2019-04-30 13:12kgvDescription Updatedbug_revision_view_page.php?rev_id=21119#r21119
2019-04-30 15:29gitNote Added: 0084080
2019-04-30 15:43tizmayloNote Added: 0084082
2019-05-01 23:41gitNote Added: 0084100
2019-05-02 15:05kgvNote Added: 0084102
2019-05-06 06:57gitNote Added: 0084177
2019-05-06 07:50abvNote Added: 0084178
2019-05-06 07:50abvAssigned Toabv => kgv
2019-05-06 11:46gitNote Added: 0084183
2019-05-06 11:50tizmayloNote Added: 0084184
2019-05-06 11:57kgvNote Added: 0084185
2019-05-06 11:57kgvAssigned Tokgv => bugmaster
2019-05-06 11:57kgvStatusresolved => reviewed
2019-05-06 12:16tizmayloNote Added: 0084186
2019-05-06 12:26gitNote Added: 0084190
2019-05-06 12:27abvNote Added: 0084191
2019-05-06 14:27bugmasterNote Added: 0084194
2019-05-06 14:27bugmasterStatusreviewed => tested
2019-05-06 14:27bugmasterTest case number => Not required
2019-05-07 08:17bugmasterNote Added: 0084207
2019-05-07 08:17bugmasterAssigned Tobugmaster => kgv
2019-05-07 08:17bugmasterStatustested => assigned
2019-05-07 08:59kgvNote Added: 0084208
2019-05-07 08:59kgvNote Edited: 0084208bug_revision_view_page.php?bugnote_id=84208#r21155
2019-05-07 10:06gitNote Added: 0084210
2019-05-07 10:07kgvNote Added: 0084211
2019-05-07 10:07kgvAssigned Tokgv => bugmaster
2019-05-07 10:07kgvStatusassigned => resolved
2019-05-07 10:07kgvStatusresolved => reviewed
2019-05-07 13:10bugmasterNote Added: 0084221
2019-05-07 14:30bugmasterStatusreviewed => tested
2019-05-08 12:38bugmasterChangeset attached => occt master 77bc2ad1
2019-05-08 12:38bugmasterStatustested => verified
2019-05-08 12:38bugmasterResolutionopen => fixed
2019-05-08 13:41gitNote Added: 0084240
2019-05-08 13:41gitNote Added: 0084242
2019-05-08 13:41gitNote Added: 0084244

Notes
(0084054)
git   
2019-04-29 19:58   
Branch CR30683 has been created by kgv.

SHA-1: eb7b8c5c584f7e1dd4a7b0229c55c20d4a711edf


Detailed log of new commits:

Author: kgv
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.
(0084061)
git   
2019-04-29 22:48   
Branch CR30683 has been updated forcibly by kgv.

SHA-1: 6b817dd589e3263ba02e5d4833ae0d1c0d9d9870
(0084063)
kgv   
2019-04-30 08:08   
Patch is ready for review.

http://jenkins-test-12.nnov.opencascade.com:8080/view/CR30683-master-KGV/ [^]
(0084080)
git   
2019-04-30 15:29   
Branch CR30683 has been updated forcibly by kgv.

SHA-1: 69acbbe50d6771d951e185b496b8102d7a8ace75
(0084082)
tizmaylo   
2019-04-30 15:43   
Looks like there are some problems with your code changes:
 
TopoDS_Shape (T2&& theOther, typename std::enable_if<opencascade::is_base_but_not_same<TopoDS_Shape, 
T2>::value>::type* = nullptr)

 
std::enable_ifopencascade::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").
(0084100)
git   
2019-05-01 23:41   
Branch CR30683 has been updated forcibly by kgv.

SHA-1: 88c60b41189aa5da923a915e2e48db3f6509eb8d
(0084102)
kgv   
2019-05-02 15:05   
> 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.
(0084177)
git   
2019-05-06 06:57   
Branch CR30683 has been updated by abv.

SHA-1: d15ff23d67c4a95db7733f04bc9702079676c5dc


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.

(0084178)
abv   
2019-05-06 07:50   
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.
(0084183)
git   
2019-05-06 11:46   
Branch CR30683_1 has been created by kgv.

SHA-1: 1c0950491bc19d404b19e48506d8e7e5f3d2b759


Detailed log of new commits:

Author: kgv
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.
(0084184)
tizmaylo   
2019-05-06 11:50   
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 [^]
(0084185)
kgv   
2019-05-06 11:57   
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).
(0084186)
tizmaylo   
2019-05-06 12:16   
>> 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 [^]
(0084190)
git   
2019-05-06 12:26   
Branch CR30683_2 has been created by abv.

SHA-1: 6b5ac9069f2462e839ba8b3b7f546112da0e3b9a


Detailed log of new commits:

Author: kgv
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.
(0084191)
abv   
2019-05-06 12:27   
Please take branch CR30683_2 for integration: it is the same as CR30683_1 except that some comments are corrected
(0084194)
bugmaster   
2019-05-06 14:27   
Combination -
OCCT branch : CR30683
master SHA - d15ff23d67c4a95db7733f04bc9702079676c5dc
d67d4b811012eef8913d3c535c29654d0acf3c4c
Products branch : master SHA - 9af78c86e3d504546d01ae29b7a2f3d86f9da763
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
(0084207)
bugmaster   
2019-05-07 08:17   
Error on CentOS64-64

http://jenkins-test-08.nnov.opencascade.com/view/IR-WEEK18_IR-WEEK18/view/OCCT%20compile/job/IR-WEEK18_IR-WEEK18-OCCT-CentOS64-64-opt-compile/4/parsed_console/ [^]
(0084208)
kgv   
2019-05-07 08:59   
CentOS64-64:
> 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


(0084210)
git   
2019-05-07 10:06   
Branch CR30683_2 has been updated forcibly by kgv.

SHA-1: f38cd7217969c109a535b1dae7b0b1058d7c8f2e
(0084211)
kgv   
2019-05-07 10:07   
Please check compilation of updated patch on mentioned platform.
(0084221)
bugmaster   
2019-05-07 13:10   
OK after fix:
http://jenkins-test-08.nnov.opencascade.com/view/CR30683_2_master/ [^]
(0084240)
git   
2019-05-08 13:41   
Branch CR30683_2 has been deleted by inv.

SHA-1: f38cd7217969c109a535b1dae7b0b1058d7c8f2e
(0084242)
git   
2019-05-08 13:41   
Branch CR30683_1 has been deleted by inv.

SHA-1: 1c0950491bc19d404b19e48506d8e7e5f3d2b759
(0084244)
git   
2019-05-08 13:41   
Branch CR30683 has been deleted by inv.

SHA-1: d15ff23d67c4a95db7733f04bc9702079676c5dc