View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0030683Open CASCADEOCCT:Codingpublic2019-04-29 19:172019-05-08 13:41
Reporterkgv Assigned Tobugmaster  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
PlatformMacOSOS 
Product Version7.3.0 
Target Version7.4.0Fixed in Version7.4.0 
Summary0030683: Coding Rules - eliminate CLang compiler warnings -Wreturn-std-move
DescriptionCLang (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.
TagsNo tags attached.
Test case numberNot required

Relationships

related to 0030681 closedbugmaster Configuration - suppress OpenGL deprecation warning on macOS 10.14 

Activities

git

2019-04-29 19:58

administrator   ~0084054

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.

git

2019-04-29 22:48

administrator   ~0084061

Branch CR30683 has been updated forcibly by kgv.

SHA-1: 6b817dd589e3263ba02e5d4833ae0d1c0d9d9870

kgv

2019-04-30 08:08

developer   ~0084063

Patch is ready for review.

http://jenkins-test-12.nnov.opencascade.com:8080/view/CR30683-master-KGV/

git

2019-04-30 15:29

administrator   ~0084080

Branch CR30683 has been updated forcibly by kgv.

SHA-1: 69acbbe50d6771d951e185b496b8102d7a8ace75

tizmaylo

2019-04-30 15:43

developer   ~0084082

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_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").

git

2019-05-01 23:41

administrator   ~0084100

Branch CR30683 has been updated forcibly by kgv.

SHA-1: 88c60b41189aa5da923a915e2e48db3f6509eb8d

kgv

2019-05-02 15:05

developer   ~0084102

> 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.

git

2019-05-06 06:57

administrator   ~0084177

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.

abv

2019-05-06 07:50

manager   ~0084178

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.

git

2019-05-06 11:46

administrator   ~0084183

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.

tizmaylo

2019-05-06 11:50

developer   ~0084184

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

kgv

2019-05-06 11:57

developer   ~0084185

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).

tizmaylo

2019-05-06 12:16

developer   ~0084186

>> 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

git

2019-05-06 12:26

administrator   ~0084190

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.

abv

2019-05-06 12:27

manager   ~0084191

Please take branch CR30683_2 for integration: it is the same as CR30683_1 except that some comments are corrected

bugmaster

2019-05-06 14:27

administrator   ~0084194

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

bugmaster

2019-05-07 08:17

administrator   ~0084207

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/

kgv

2019-05-07 08:59

developer   ~0084208

Last edited: 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


git

2019-05-07 10:06

administrator   ~0084210

Branch CR30683_2 has been updated forcibly by kgv.

SHA-1: f38cd7217969c109a535b1dae7b0b1058d7c8f2e

kgv

2019-05-07 10:07

developer   ~0084211

Please check compilation of updated patch on mentioned platform.

bugmaster

2019-05-07 13:10

administrator   ~0084221

OK after fix:
http://jenkins-test-08.nnov.opencascade.com/view/CR30683_2_master/

git

2019-05-08 13:41

administrator   ~0084240

Branch CR30683_2 has been deleted by inv.

SHA-1: f38cd7217969c109a535b1dae7b0b1058d7c8f2e

git

2019-05-08 13:41

administrator   ~0084242

Branch CR30683_1 has been deleted by inv.

SHA-1: 1c0950491bc19d404b19e48506d8e7e5f3d2b759

git

2019-05-08 13:41

administrator   ~0084244

Branch CR30683 has been deleted by inv.

SHA-1: d15ff23d67c4a95db7733f04bc9702079676c5dc

Related Changesets

occt: master 77bc2ad1

2019-04-29 16:51:34

kgv


Committer: bugmaster Details Diff		 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.		 Affected Issues
0030683
mod - src/Standard/Standard_Handle.hxx Diff File
mod - src/TopLoc/FILES Diff File
mod - src/TopLoc/TopLoc_SListOfItemLocation.hxx Diff File
rm - src/TopLoc/TopLoc_SListOfItemLocation.lxx Diff File
mod - src/TopoDS/TopoDS_Shape.hxx Diff File

Issue History

Date Modified Username Field Change
2019-04-29 19:17 kgv New Issue
2019-04-29 19:17 kgv Assigned To => kgv
2019-04-29 19:17 kgv Description Updated
2019-04-29 19:17 kgv Relationship added related to 0030681
2019-04-29 19:58 git Note Added: 0084054
2019-04-29 22:48 git Note Added: 0084061
2019-04-30 08:08 kgv Note Added: 0084063
2019-04-30 08:08 kgv Assigned To kgv => abv
2019-04-30 08:08 kgv Status new => resolved
2019-04-30 13:10 kgv Description Updated
2019-04-30 13:10 kgv Description Updated
2019-04-30 13:12 kgv Description Updated
2019-04-30 15:29 git Note Added: 0084080
2019-04-30 15:43 tizmaylo Note Added: 0084082
2019-05-01 23:41 git Note Added: 0084100
2019-05-02 15:05 kgv Note Added: 0084102
2019-05-06 06:57 git Note Added: 0084177
2019-05-06 07:50 abv Note Added: 0084178
2019-05-06 07:50 abv Assigned To abv => kgv
2019-05-06 11:46 git Note Added: 0084183
2019-05-06 11:50 tizmaylo Note Added: 0084184
2019-05-06 11:57 kgv Note Added: 0084185
2019-05-06 11:57 kgv Assigned To kgv => bugmaster
2019-05-06 11:57 kgv Status resolved => reviewed
2019-05-06 12:16 tizmaylo Note Added: 0084186
2019-05-06 12:26 git Note Added: 0084190
2019-05-06 12:27 abv Note Added: 0084191
2019-05-06 14:27 bugmaster Note Added: 0084194
2019-05-06 14:27 bugmaster Status reviewed => tested
2019-05-06 14:27 bugmaster Test case number => Not required
2019-05-07 08:17 bugmaster Note Added: 0084207
2019-05-07 08:17 bugmaster Assigned To bugmaster => kgv
2019-05-07 08:17 bugmaster Status tested => assigned
2019-05-07 08:59 kgv Note Added: 0084208
2019-05-07 08:59 kgv Note Edited: 0084208
2019-05-07 10:06 git Note Added: 0084210
2019-05-07 10:07 kgv Note Added: 0084211
2019-05-07 10:07 kgv Assigned To kgv => bugmaster
2019-05-07 10:07 kgv Status assigned => resolved
2019-05-07 10:07 kgv Status resolved => reviewed
2019-05-07 13:10 bugmaster Note Added: 0084221
2019-05-07 14:30 bugmaster Status reviewed => tested
2019-05-08 12:38 bugmaster Changeset attached => occt master 77bc2ad1
2019-05-08 12:38 bugmaster Status tested => verified
2019-05-08 12:38 bugmaster Resolution open => fixed
2019-05-08 13:41 git Note Added: 0084240
2019-05-08 13:41 git Note Added: 0084242
2019-05-08 13:41 git Note Added: 0084244