DiscussionsIssue archiveOCCT:Foundation Classes

Archived issue #0027111

Add generalized copy constructor in handle class for old compilers

CommunityOCCT:Foundation Classesclosed34 public notes

Search issues

Description

The new handle system introduced the call ambiguity problem for compilers w/o default template parameters supported. The solutions listed here http://dev.opencascade.org/index.php?q=node/1091 require massive code changes. To avoid them the co-called generalized copy constructor could be implemented.
For now it will not solve the call ambiguity problem as there is also the upcast to non-const reference operator, which is required for current OCC code to build. But taking into account this issue #0026377 it's supposed to remove this operator.

The proposed ctor is expected to be enabled only for old compilers, allowing to suppress unneeded copy in modern compilers.

Steps to reproduce

N/A

Additional information

The similar approach is used in std::shared_ptr and boost::shared_ptr to allow construction from smart pointer of convertible type.

Public activity

34 archived notes

Participants are labeled by their role within this record.

01Commenter 2
Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Alexander Zashivalov
Date: Mon Jan 25 20:16:58 2016 +0300

    0027111: Add generalized copy constructor in handle class for old compilers

02Author
WARNING: the changes in pushed branch will generate some warnings about returning local variables from functions. They ARE reasonable and will make many tests to throw exceptions. The fix is straightforward and deferred until the resolution from OCC team about whole conception.
03Commenter 3
Adding a generalized template constructor will have two major benefits:

1. Easier user code porting to OCC 7.0 for pre-vc12 compilers (where SFINAE used for upcast operator does not work)

2. Better compliance with the std::/boost:: shared_ptr.
04Commenter 4
That is logical, why not. Just two things:

- make sure the code compiles and works fine (by the time I tried this my impression was it would be difficult to get rid of "returning temporaries"; however this might be caused by WOK/CDL issues by the time, or just wrong)

- check that performance impact is not too big
05Commenter 2
Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Alexander Zashivalov
Date: Mon Jan 25 20:16:58 2016 +0300

    0027111: Add generalized copy constructor in handle class for old compilers
06Author
The new branch is pushed. It compiles w/o errors/warnings. Publicity available tests showed no regressions or performance drops.
07Commenter 7
I believe that line 77 in Standard_Handle.hxx:

#if !TEMPLATE_DEFAULT_ARGS_SUPPORTED

will expand to

#if !(defined(__clang__)) || (defined(__INTEL_COMPILER) && __INTEL_COMPILER >= 1300) || \
+ (defined(_MSC_VER) && _MSC_VER >= 1800) || \
+ (defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)))

and thus it will apply to all compilers except clang, which is clearly incorrect. Why not just replacing cast operators by copy constructors in the same place, without introducing new macro?
08Commenter 2
Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Alexander Zashivalov
Date: Wed Jan 27 12:14:18 2016 +0300

    0027111: Add generalized copy constructor in handle class for old compilers
09Author
Yes, sorry. I've been using that macro during tests with forced 0 or 1, so didn't catch the error. New branch, containing your suggestion about eliminating the macro, is pushed.
10Commenter 10
Alexander, can you explain why you needed to introduce occ_is_base_of template? Would not it work with using standard tools? If the problem appears while building OCCT (i.e. not in your product), please specify what configuration fails. If the problem occurs on your code, could you provide reproducer? Thank you in advance!

I shall be able to test performance impact of this change during the next week, and then we can agree on how to do.
11Author
Sure. I've been trying to build OCC master branch with visual studio 2010. With generalized ctor enabled many files were not able to compile with the errors like this:
error C2139: 'Units_Quantity' : an undefined class is not allowed as an argument to compiler intrinsic type trait '__is_base_of'
...
see reference to class template instantiation 'std::tr1::is_base_of<_Base,_Der>' being compiled with
[
_Base=Units_Quantity,
_Der=Units_Quantity
]

This happens because of forward declaration of class Units_Quantity, while is_base_of requires complete types. Including headers with complete types leads to many cyclic dependencies, which is hard to resolve and may require massive code changes. That's why I wrote a simple trait wrapper with specialization for is_same types to be always true.
Another thing to mention. If the above explanation is correct, then why there is no such issue with cast operators? They do use std::is_base_of. This is tied to template instantiation order. When we call handle<MyClass> only __declarations__ of it's methods are actually instantiated, not definitions. For both, modern and old compilers, cast operators have valid deduced declarations, thus their enable_ifs' deducing is deferred until actual call. But this is not true for our ctor as it has non-deduced context (enabler with is_base_of) in signature. In order to acquire a valid declaration the compiler performs this context deducing, which involves instantiating of is_base_of.
Actually we can deffer this instantiation by removing default parameter nullptr. But this will exclude ctor from overload resolution and we will see "no user-defined conversion" errors.
Hope I made things clearer. Would be glad to hear if you have any suggestions about how this whole thing can be implemented.
12Commenter 12
Alexander, thank you for detailed explanation! I recall I encountered this kind of problems while working on handles this year, and your solution seems to be quite good workaround against that.

I have tested your branch and confirm that it builds and does not show any performance degradation. However, I have strong doubt that the change actually resolves an original issue (ambiguity). In my tests, ambiguity is still there when the object being passed is non-const handle -- in that case, cast to non-const reference (subject of #0026377) is used instead of constructor. Actually, all ambiguities I have removed from OCCT seem to be in place -- I see them after reverting corresponding commit.
13Author
Andrey, this solution, unfortunately, is supposed to work only for const references. I.e. to solve ambiguity call problems in situations like:

foo (const Handle_Geom_Curve& theCurve);
foo (const Handle_Geom_Surface& theSurface);
// ...
Handle_Geom_BSplineCurve aCurve;
foo (aCurve);

This doesn't work for non-const references, cuz binding rvalues (temporaries created from copy ctor) is prohibited by the C++ standard (http://stackoverflow.com/questions/13826897/why-not-non-const-reference-to-temporary-objects). Also, as I've mentioned in issue description, there is no room for both, generalized copy ctor and non-const reference cast. Because it's OK for compiler to take non-const cast and add const qualifier. But as I understand, the non-const reference cast is going to be removed. Instead, the SafeUpCast() method will be provided. This is when the copy ctor will work to resolve call ambiguities.
Anyway this is the same behavior as with any other smart pointers. If you really need to pass a non-const reference, then you have to either create lvalue pointer with the exact type, or cast it to the exact type in call context.

As I can see, you've already made a big work to adopt OCC codebase to the new handles system, in particular to resolve call ambiguities. So the proposed copy ctor may not be of a big help here. But it definitely could help OCC users as they will have the same behavior for handle, as for, say, shared_ptr. As an example, we had to change only small amount of our code to successfully build VC10 projects. W/o generalized copy ctor this would take much more effort as we have many calls like in code snippet above.
14Commenter 14
Alexander, I see your arguments; just one remark is that on your example your current fix will still show ambiguity due to non-const reference cast applied to aCurve.

Unfortunately, approach I have tried with SafeUpCast() method does not work, and I see no way to make it working (I can share a branch if you wish to check this). This means that we cannot replace non-const reference cast by some similar but correct code, and the only way to make the code correct is to pass handle of exact type. This requires more changes than just using SafeUpCast() method, and hence I doubt if it is worth making this change in OCCT 7.
15Author
Andrey, yes, I understand that currently non-const upcast operator prevent copy ctor from working. This is because they both are treated as user-defined conversions. Personally I've just commented it out during builds. I was hoping that SafeUpCast() should work as expected, sadly to hear that it doesn't.
Well then, the proposed code integration seems to be useless for now and should be deferred until non-const cast operator removed. We will think about another alternatives and let you know about new findings.
16Commenter 2
Branch [archived branch] has been created by Participant.

[revision removed]


Detailed log of new commits:

Author: abv
Date: Mon Jan 25 10:14:20 2016 +0300

    0027104: DownCast() cannot return null for mismatched handle
    
    Method DownCast() is amended to be available only when argument is actually a pointer or handle to a base class.
    When DownCast() is used for the same type, derived, or unrelated class (i.e. where there is no actual down casting), compiler error will be generated.
    
    OCCT code is updated to remove such trivial DownCast()s; a few places where DownCast() was used with argument of unrelated type are corrected.
    
    DRAW command QAHandleCast is removed (it was useful only during redesign of handles).

Author: abv
Date: Tue Aug 11 23:01:05 2015 +0300

    0026549: Provide move constructors and operators for basic classes
    
    Move constructor and operator added for opencascade::handle<>

Author: abv
Date: Sat Feb 6 15:26:26 2016 +0300

    0027111: Add generalized copy constructor in handle class for old compilers
    
    Copy constructor and assignment operator from handle of derived type is added in handle class.
    They are enabled only if macro OCC_HANDLE_NOCASTS is defined, and operators of cast of handle to reference to handle to base type are disabled in that case.
    
    Useless type casts to handle to base type are removed in GC and GCE2d classes.
    Code returning reference to handle from function is corrected to return it either by value or as reference to handle of actual type.

Author: abv
Date: Thu Feb 4 07:12:03 2016 +0300

    0026377: Passing Handle objects as arguments to functions as non-const reference to base type is dangerous
    
    Operator of cast to non-const reference is declared deprecated to produce compiler warning if used (usually implicitly).
    
    OCCT code is updated to avoid that cast, occurring when function accepting non-const reference to handle is called with handle to derived type. For that, local variable of argument type is passed instead, and down-cast is used to get it to desired type after the call.

Author: Commenter 4
Date: Sat Jan 30 15:51:35 2016 +0300

    0027118: Configuration - do not suppress deprecation warnings when using msvc
    
    Patch removes option -wd4996 from VS project settings (which suppresses old deprecation warnings).
    Instead, macros _CRT_SECURE_NO_WARNINGS (suppresses 444 warnings) and _CRT_NONSTDC_NO_DEPRECATE (suppresses 17 warnings) have been added.
    
    Deprecation warning on GetVersionEx() has been suppressed locally in OSD_Host.cxx.
    In STEPConstruct_AP203Context.cxx, OSD_Host is used instead of low-level system functions.
    This eliminates dependency of TKSTEP on winsock32.lib on Windows.

Author: Commenter 4
Date: Sat Jan 30 14:54:12 2016 +0300

    0027113: Coding - add macros Standard_DEPRECATED for marking deprecated functionality

17Commenter 17
I have been able to build a version comprising several pending fixes on Hendles which finally seems to me consistent, see branch CR27111_4.

The default behavior did not change (except that non-const cast of handle should generate deprecation warning on VC10-11, and that DownCast() is not available for unrelated types). However, compiler-time macro OCCT_HANDLE_NOCAST can be used to trigger the implementation from casts to constructors. This should allow getting rid of ambiguity problem and other troubles caused by cast operators (see Upgrade guide), but the code must be modified to avoid use of non-const casts.

Please review.
18Commenter 2
Branch [archived branch] has been created by Participant.

[revision removed]


Detailed log of new commits:

Author: abv
Date: Mon Jan 25 10:14:20 2016 +0300

    0027104: DownCast() cannot return null for mismatched handle
    
    Method DownCast() is amended to be available only when argument is actually a pointer or handle to a base class.
    When DownCast() is used for the same type, derived, or unrelated class (i.e. where there is no actual down casting), compiler error will be generated.
    
    OCCT code is updated to remove such trivial DownCast()s; a few places where DownCast() was used with argument of unrelated type are corrected.
    
    DRAW command QAHandleCast is removed (it was useful only during redesign of handles).

Author: abv
Date: Tue Aug 11 23:01:05 2015 +0300

    0026549: Provide move constructors and operators for basic classes
    
    Move constructor and operator added for opencascade::handle<>

Author: abv
Date: Sat Feb 6 15:26:26 2016 +0300

    0027111: Add generalized copy constructor in handle class for old compilers
    
    Copy constructor and assignment operator from handle of derived type is added in handle class.
    They are enabled only if macro OCC_HANDLE_NOCASTS is defined, and operators of cast of handle to reference to handle to base type are disabled in that case.
    
    Useless type casts to handle to base type are removed in GC and GCE2d classes.
    Code returning reference to handle from function is corrected to return it either by value or as reference to handle of actual type.

Author: abv
Date: Thu Feb 4 07:12:03 2016 +0300

    0026377: Passing Handle objects as arguments to functions as non-const reference to base type is dangerous
    
    Operator of cast to non-const reference is declared deprecated to produce compiler warning if used (usually implicitly).
    
    OCCT code is updated to avoid that cast, occurring when function accepting non-const reference to handle is called with handle to derived type. For that, local variable of argument type is passed instead, and down-cast is used to get it to desired type after the call.
19Commenter 2
Branch [archived branch] has been created by Participant.

[revision removed]


Detailed log of new commits:

Author: Commenter 1
Date: Sun Feb 14 15:01:50 2016 +0300

    Adding macros OCCT_HANDLE_NOCAST
20Commenter 2
Branch [archived branch] has been created by Participant.

[revision removed]


Detailed log of new commits:

Author: abv
Date: Mon Jan 25 10:14:20 2016 +0300

    0027104: DownCast() cannot return null for mismatched handle
    
    Method DownCast() is made template, to be available only when argument is actually a pointer or handle to a base class.
    
    For compatibility with existing code, method DownCast() that can be used for the same type, derived, or unrelated class (i.e. where there is no actual down casting) is still available, its use shall cause "deprecated" compiler warning.
    
    OCCT code is updated to remove meaningless DownCast()s; a few places where DownCast() was used with argument of unrelated type are corrected.
    
    DRAW command QAHandleCast is removed (it was useful only during redesign of handles).

Author: abv
Date: Tue Aug 11 23:01:05 2015 +0300

    0026549: Provide move constructors and operators for basic classes
    
    Move constructor and operator added for opencascade::handle<>

Author: abv
Date: Sat Feb 6 15:26:26 2016 +0300

    0027111: Add generalized copy constructor in handle class for old compilers
    
    Copy constructor and assignment operator from handle of derived type is added in handle class.
    They are enabled only if macro OCC_HANDLE_NOCASTS is defined, and operators of cast of handle to reference to handle to base type are disabled in that case.
    
    Useless type casts to handle to base type are removed in GC and GCE2d classes.
    Code returning reference to handle from function is corrected to return it either by value or as reference to handle of actual type.

Author: abv
Date: Thu Feb 4 07:12:03 2016 +0300

    0026377: Passing Handle objects as arguments to functions as non-const reference to base type is dangerous
    
    Operator of cast to non-const reference is declared deprecated to produce compiler warning if used (usually implicitly).
    
    OCCT code is updated to avoid that cast, occurring when function accepting non-const reference to handle is called with handle to derived type. For that, local variable of argument type is passed instead, and down-cast is used to get it to desired type after the call.
21Commenter 2
Branch [archived branch] has been updated forcibly by Participant.

[revision removed]
22Commenter 4
No remarks.
23Commenter 2
Branch [archived branch] has been updated by Participant.

[revision removed]


Detailed log of new commits:

Author: abv
Date: Wed Feb 17 16:08:01 2016 +0300

    fix regressions

24Commenter 2
Branch [archived branch] has been updated forcibly by Participant.

[revision removed]
25Commenter 2
Branch [archived branch] has been updated forcibly by Participant.

[revision removed]
26Commenter 2
Branch [archived branch] has been updated forcibly by Participant.

[revision removed]
27Commenter 1
Branch CR27111_6 includes fixes for 26549,27104,26337,27118 and 27113
28Commenter 2
Branch [archived branch] has been deleted by Commenter 4.

[revision removed]
29Commenter 2
Branch [archived branch] has been deleted by Commenter 4.

[revision removed]
30Commenter 2
Branch [archived branch] has been deleted by Commenter 4.

[revision removed]
31Commenter 2
Branch [archived branch] has been deleted by Commenter 4.

[revision removed]
32Commenter 2
Branch [archived branch] has been deleted by Commenter 4.

[revision removed]
33Commenter 2
Branch [archived branch] has been deleted by Commenter 4.

[revision removed]
34Commenter 2
Branch [archived branch] has been deleted by Commenter 4.

[revision removed]

Related records