MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0030683Open CASCADE[OCCT] OCCT:Codingpublic2019-04-29 19:172019-05-08 13:41
Reporterkgv 
Assigned Tobugmaster 
PrioritynormalSeverityminor 
StatusverifiedResolutionfixed 
PlatformMacOSOSOS VersionX
Product Version[OCCT] 7.3.0 
Target Version[OCCT] 7.4.0Fixed in Version 
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
Attached Files

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

-  Notes
(0084054)
git (administrator)
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 (administrator)
2019-04-29 22:48

Branch CR30683 has been updated forcibly by kgv.

SHA-1: 6b817dd589e3263ba02e5d4833ae0d1c0d9d9870
(0084063)
kgv (developer)
2019-04-30 08:08

Patch is ready for review.

http://jenkins-test-12.nnov.opencascade.com:8080/view/CR30683-master-KGV/ [^]
(0084080)
git (administrator)
2019-04-30 15:29

Branch CR30683 has been updated forcibly by kgv.

SHA-1: 69acbbe50d6771d951e185b496b8102d7a8ace75
(0084082)
tizmaylo (developer)
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 (administrator)
2019-05-01 23:41

Branch CR30683 has been updated forcibly by kgv.

SHA-1: 88c60b41189aa5da923a915e2e48db3f6509eb8d
(0084102)
kgv (developer)
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 (administrator)
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 (manager)
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 (administrator)
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 (developer)
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 (developer)
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 (developer)
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 (administrator)
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 (manager)
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 (administrator)
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 (administrator)
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 (developer)
2019-05-07 08:59
edited on: 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 (administrator)
2019-05-07 10:06

Branch CR30683_2 has been updated forcibly by kgv.

SHA-1: f38cd7217969c109a535b1dae7b0b1058d7c8f2e
(0084211)
kgv (developer)
2019-05-07 10:07

Please check compilation of updated patch on mentioned platform.
(0084221)
bugmaster (administrator)
2019-05-07 13:10

OK after fix:
http://jenkins-test-08.nnov.opencascade.com/view/CR30683_2_master/ [^]
(0084240)
git (administrator)
2019-05-08 13:41

Branch CR30683_2 has been deleted by inv.

SHA-1: f38cd7217969c109a535b1dae7b0b1058d7c8f2e
(0084242)
git (administrator)
2019-05-08 13:41

Branch CR30683_1 has been deleted by inv.

SHA-1: 1c0950491bc19d404b19e48506d8e7e5f3d2b759
(0084244)
git (administrator)
2019-05-08 13:41

Branch CR30683 has been deleted by inv.

SHA-1: d15ff23d67c4a95db7733f04bc9702079676c5dc

- Related Changesets
occt: master 77bc2ad1
Timestamp: 2019-04-29 16:51:34
Author: 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.
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 View Revisions
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 View Revisions
2019-04-30 13:10 kgv Description Updated View Revisions
2019-04-30 13:12 kgv Description Updated View Revisions
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 View Revisions
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


Copyright © 2000 - 2019 MantisBT Team
Powered by Mantis Bugtracker