Open CASCADE Technology 7.0 preview

Forums: 

Dear All,

We have finalized development of several major new features for Open CASCADE Technology 7.0.
The most essential changes -- removal of CDL and WOK -- have been raised to master branch of the OCCT Git repository last Friday, and we propose this version for public review, as "alpha" version of OCCT 7.0.

This article is aimed to give overview of the major changes already integrated, and give idea of the effort required to port existing code on the new version.

Preface

As a reminder, CDL (Cas.Cade Declaration Language) and WOK (OCCT build environment) are legacy technologies used since the first days of Cas.Cade to organize its development. They provided features not-yet well supported by C++ compilers at that time (1990x), such as smart pointers (OCCT Handles), run-time type identification, and templates (CDL generics). The problem is that CDL and WOK require extra effort for their maintaining and restrict possibilities of OCCT developers; as well, the need to learn and use these tools impedes adoption of OCCT by new users and developers. The goal of the refactoring project started in 2013 is reimplementing features of OCCT, based on CDL and WOK, via use of modern technologies and tools - C++ templates metaprogramming for code generation, and CMake for build system. This development has been driven by ambitions to:

  • Reduce amount of OCCT source code due to elimination of trivial code generated by WOK.
  • Keep all useful documentation comments contained in CDL files, by translating them to Doxygen-style comments in C++ headers.
  • Preserve high level of compatibility with previous versions, making no change in architecture, and minimal possible changes in interfaces and behavior.
  • Facilitate upgrade of applications to OCCT 7.0 by automation of necessary changes, and providing detailed description of known possible issues and ways to resolve these.

This work took considerable time, and now we are happy to share the result with the community. The following paragraphs provide detailed description of the major changes already done and in progress (including those not related to CDL and WOK).

Elimination of CDL

Related issue: #24002. All classes previously defined using CDL language have been converted into pure C++ format (defined in C++ header files). This enables use of modern C++ features in whole OCCT code, and makes WOK unnecessary. As before, all sources are contained in src subfolder. Subfolder inc is populated by header files automatically during building and installation. Code previously generated in subfolder drv, as well as header files with names starting with "Handle_.." (containing definition of Handle classes), has been removed or merged into sources. This reduces number of source files in OCCT source archive by about 19.000, and saves ~22 MB of disk space:

Version Number of files Disk space, MB
src inc drv src inc drv
6.9.0 12 918 12 952 15 458 85.6 33.9 17.5
7.0.0 14 321 8 189 0 86.6 28.4 0

Use of CMake build system

Related issue: #25114. WOK is not necessary anymore for building OCCT from bare sources (though it still can be used in traditional way -- auxiliary files required for that are preserved). CMake is now considered as main build system for OCCT. As compared to CMake build scripts in OCCT 6.9.0, it introduces the following improvements (mostly useful for Windows developers):

  • Does not require generation of CMake scripts by WOK: all CMake files are included with OCCT sources.
  • Subfolder inc in build directory is populated not by copies of header files, by but short-cuts, redirecting to same-named header located in src. This allows having single inc directory without duplication of header files, thus reducing chance of losing modifications made to wrong copy of the header.
  • When only part of OCCT is configured for building, only relevant includes will be installed.
  • Header files are included in Visual Studio projects.
  • Several build configurations can coexist in the same Visual Studio projects (Release, Debug, and others - as supported by CMake).
  • Launch of DRAWEXE is possible directly from Visual Studio.
  • Project for generation of overview documentation is added, making documentation sources easily accessible from Visual Studio IDE.

As a fallback solution for users not wishing to use CMake, we extracted generator of Visual Studio project files from WOK so it is available in OCCT code repository (genproj.bat), and are going to include traditionally organized project files in source deliveries of OCCT releases.

Conversion of TCollection classes

Related issue: #24750. Instantiations of generic classes from TCollection package have been converted into equivalent instances of NCollection templates. The names of the affected classes (such as TColgp_Array1OfPnt) are preserved, thus the code that used these classes should work as before. However, that code might need to be amended to take into account the following:

  • Forward declarations of collection classes such as class TColgp_Array1OfIPnt; are not valid anymore; they should be replaced by inclusion of corresponding header (#include <TColgp_Array1OfPnt.hxx>).
  • Equivalents of methods Find1() and ChangeFind1() in TCollection_DataMap have different names in NCollection_DataMap: Seek() and ChangeSeek(), respectively.
  • Overloaded functions differing only by accepting equivalent TCollection and NCollection classes now become equal. Such code should be corrected to keep only one definition.

New type system

Related issue: #24947. OCCT legacy type system has been re-implemented using C++ templates. Macro STANDARD_TYPE() previously expanding to a call to a global function now expands to call to template method of the Standard_Type class. Use of templates eliminates necessity of explicit instantiation of type definition in C++ code, thus macros IMPLEMENT_DOWNCAST, IMPLEMENT_STANDARD_* are not necessary anymore. Note that these macros are still defined (as empty) for compatibility reasons. It is still necessary to include macro DEFINE_STANDARD_RTTI(Class,Base) in definition of the class to make it known to the OCCT type system. That macro defines method DynamicType(), returning its type descriptor, and auxiliary method get_type_name() returning bare name of the class. Note second argument to the macro, name of the base class, which was was not present in previous versions. Note that OCCT RTTI is not limited to classes inheriting Standard_Transient, and can be applied to any class; void should be used as second argument to DEFINE_STANDARD_RTTI macro if parent class is absent or does not provide OCCT RTTI. Example:

  struct ClassA
  {
    DEFINE_STANDARD_RTTI(ClassA, void)
  };

  struct ClassB : public ClassA
  {
    DEFINE_STANDARD_RTTI(ClassB, ClassA)
  }; 

  ClassA* aPtr = …;
  if (aPtr->DynamicType()->Parent() == STANDARD_TYPE(ClassA))
  {
    // object pointed by aPtr is first descendant of ClassA
  }

As before, OCCT RTTI does not support multiple inheritance. For iteration by class hierarchy now method Standard_Type::Parent() should be used; class Standard_AncestorIterator has been removed. This change leads to slight increase of memory allocated by OCCT at load time (by ~450 KB in total for all modules).

Template handles

Related issue: #24023. Handles are now implemented as template class opencascade::handle<>. Macro Handle() is still defined for compatibility, it expands to opencascade::handle<arg>. New implementation provides the following benefits:

  • There is no need to use macro DEFINE_STANDARD_HANDLE to define a handle: it can be used without any additional declarations. Note however that macro DEFINE_STANDARD_HANDLE is still present and used in OCCT headers; it defines typedef to handle with the name corresponding to name of the class prefixed by "Handle_". This is done with hope to have better compatibility with existing code.
  • There is no need to use OCCT type system for the class to use handle with it; you can use Handle() for any class inheriting Standard_Transient (method DownCast() is implemented using standard C++ RTTI).

In result, the code builds in less time, produces binaries of less size, and runs faster:

Version Build time Binaries size All tests execution time (sequential)
master 30 min 67 MB 7 hours 7 min
new handles 28 min 62 MB 6 hours 59 min

The implementation of opencascade::handle<> is similar to boost::intrusive_ptr<>, with interface adapted to keep existing code using OCCT handles working. In particular, methods to cast Handle object to reference to Handle to base class are provided. This allows passing Handles to functions accepting such references without copying, as before. Current implementation requires that compiler provides support of some C++11 features. It is tested to work with:

  • GCC 4.3 and above; compiler option -std=c++11 should be used (-std=c++0x in GCC 4.3-4.6).
  • Visual C++ 10 (Visual Studio 2010) and above; lower versions of VC++ are not supported.
  • CLang 3.4, compiler option -std=c++11 should be used.

New implementation of Handle has some implications affecting their usage in existing code. The following issues have been encountered during porting OCCT and other projects, and can be faced in porting other applications designed to work with previous versions of OCCT:

Forward declarations of Handle classes

Forward declarations of Handle classes as class are not valid anymore, and should be removed. In most cases, forward declaration of argument class is sufficient. Example:

class Handle(Geom_Curve); // compiler error: opencascade::handle<> is a template

class Geom_Curve; // sufficient for using Handle(Geom_Curve)
More #include statements may be required

Use of handle objects (construction, comparison using operators == or !=, use of method DownCast()) now requires that type of the object pointed by Handle is completely known at compile time. Thus it may be necessary to include header of the corresponding class to make the code compilable. For example, the following lines will fail to compile if Geom_Line.hxx is not included:

Handle(Geom_Line) aLine = 0;
if (aLine != aCurve) {...} 
if (aCurve->IsKind(STANDARD_TYPE(Geom_Line)) {...}
aLine = Handle(Geom_Line)::DownCast (aCurve);

Note that comparison of handles of incompatible types will now cause compile-time error:

Handle(Geom_Curve) aCurve;
Handle(Geom_Surface) aSurface;
if (aCurve == aSurf) {...} // compiler error
Misuse of Handle() macro

In OCCT versions before 7.0, Handle(C) macro was expanded to Handle_C. Sometimes this was used to generate names for typedefs, and now such code becomes not compilable, e.g.:

typedef NCollection_Handle Handle(Message_Msg);

Another case when macro Handle() is not properly used is when it's argument is not just a class name, but longer expression, for instance (note misplaced closing parenthesis):

aBSpline = Handle(Geom2d_BSplineCurve::DownCast(BS->Copy()));

This code will not compile anymore; it should be manually corrected.

Use of Handle in conditional expression

Edit 21.08.2015: this paragraph is not relevant anymore; operator of cast of Handle to bool is provided

Handle does not provide cast operator to pointer or to bool, thus cannot be used in conditional expressions. Method IsNull() should be used for checking Handle to be non-zero. Example:

Handle(Geom_Curve) aCurve = …;
if (aCurve) {...} // compiler error
if (! aCurve.IsNull()) {...} // ok
Ambiguity of calls to overloaded functions

This issue appears in compilers that do not support default arguments in template functions (known cases are MSVC++ 11 and below): compiler reports ambiguity error if handle is used in argument of call to function that has two or move overloaded versions, accepting handles to different types. The problem is that operator const handle<T2>& is defined for any type T2, thus compiler cannot make a right choice. Example:

void func (const Handle(Geom_Curve)&);
void func (const Handle(Geom_Surface)&);

Handle(Geom_TrimmedCurve) aCurve = new Geom_TrimmedCurve (...);
func (aCurve); // ambiguity error in VC++ 10

To resolve this ambiguity, change your code so that argument type correspond exactly to the function signature. In some cases this can be done by using relevant type for the corresponding variable, like in the example above:

Handle(Geom_Curve) aCurve = new Geom_TrimmedCurve (...);  

Other variants are assigning the argument to local variable of correct type, using direct cast or constructor:

const Handle(Geom_Curve)& aGCurve (aTrimmedCurve);
func (aGCurve); // OK - argument has exact type
func (static_cast(aCurve)); // OK - direct cast 
func (Handle(Geom_Curve)(aCurve)); // OK - temporary handle is constructed

Another possibility is defining additional templated variant of the overloaded function causing ambiguity, and use SFINAE to resolve the ambiguity. For example of this technique, see definition of the template variant of the method IGESData_IGESWriter::Send().

Lack of implicit cast to base type

Due to the fact that now cast of handle to reference to handle to the base type is user-defined operation, conversions that require this cast combined with other user-defined cast will not be resolved automatically by compiler. For example:

Handle(Geom_Curve) aC = GCE2d_MakeSegment(p1, p2); // compiler error

The problem here is that class GCE2d_MakeSegment has user-defined conversion to const Handle(Geom_TrimmedCurve)&, which is not the same as type of the local variable aC. To resolve this, use method Value():

Handle(Geom_Curve) aC = GCE2d_MakeSegment(p1, p2).Value(); // ok

or use variable of appropriate type:

Handle(Geom_TrimmedCurve) aC = GCE2d_MakeSegment(p1, p2); // ok
C-style casts of pointers and references to Handle objects

C-style cast of Handle object to reference or pointer to handle of derived type are not allowed anymore; cast to base type is possible but will cause compiler warnings in GCC ("use of type punned pointer breaks C strict aliasing rules"). Such casts should be replaced by calls to DownCast() method. Example:

Handle(Geom_Curve) aCurve = …;
const Handle(Geom_Line)& aLine = (Handle(Geom_Line)&)aCurve; // compiler error
const Handle(Geom_Line)& aLine = *((Handle(Geom_Line)*)&aCurve); // GCC warning

Handle(Geom_Line) aLine = Handle(Geom_Line)::DownCast (aCurve); // OK
References to temporary objects

In previous versions, compiler was able to detect situation when local variable of reference type to Handle is initialized by temporary object, and ensured that lifetime of that object is longer than that of the variable. Since OCCT 7.0, it will not work if types of the temporary object and variable are different (due to involvement of user-defined type cast), thus such temporary object will be destroyed immediately. Example:

// note that DownCast() returns new temporary object!
const Handle(Geom_BoundedCurve)& aBC =
Handle(Geom_TrimmedCurve)::DownCast(aCurve);
aBC->Transform (T); // access violation in OCCT 7.0

This kind of errors can be detected only at run-time.

Separation of BSpline cache

Related issue: #24682. Implementation of NURBS curves and surfaces has been revised: cache of polynomial coefficients, used to accelerate calculate values of B-spline, is separated from data objects (Geom2d_BSplineCurve, Geom_BSplineCurve, Geom_BSplineSurface), into dedicated classes (BSplCLib_Cache and BSplSLib_Cache). The benefits of this change are:

  • Reduced memory footprint of OCCT shapes (up to 20% on some cases)
  • Possibility to evaluate the same B-Spline concurrently in parallel threads without data races and mutex locks

The drawback is that direct evaluation of B-Splines using methods of curves and surfaces becomes slower, due to absence of cache. The way to avoid slow down is to use adaptor classes (Geom2dAdaptor_Curve, GeomAdaptor_Curve and GeomAdaptor_Surface): they now use cache when the curve or surface is a B-spline. OCCT algorithms are changed to use adaptors for B-spline calculations instead of direct methods of curves and surfaces. This process is not yet completely finalized, and some operations in current version of OCCT master branch are still slower than in OCCT 6.9.0 for this reason. This should be corrected before OCCT 7.0 release. The same changes (use of adaptors instead of direct call to curve and surface methods) should be implemented in relevant places in applications based on OCCT in order to get maximum performance.

Removal of obsolete functionality

Related issues: #24927, #25148. Some obsolete code will be removed in OCCT 7.0. As previously announced, already removed components are old persistence schemas and TKNIS visualization toolkit. Other candidates for removal are old Boolean operations (ones provided by TKBool toolkit).

Use direct link instead of plugin mechanism for OCAF persistence

Related issue: #25812. This change is not yet included in master branch, it should be completed in the coming month. The goal is to change current complicated mechanism of loading OCAF persistence toolkits as plugins, that requires setting of numerous environment variables, depends on resource files, and can be broken easily, by direct linking of the necessary library on application level.

Upgrade tools

Related issue: #24816. To facilitate porting existing code on OCCT 7.0, an automatic upgrade tool (Tcl script) is being developed. The detailed description of this script and recommendations on its usage to upgrade applications based on OCCT will be available soon, as part of the Upgrade Guide

Further steps

Current version of code is considered as ready for review and trial, and we will be happy to get feedback from the community regarding these changes. Further we are going to upgrade several existing applications of the new version, to verify the upgrade process and elaborate more detailed documentation. In September we are going to make a check point to consolidate obtained experience and community inputs, and define the next steps. We are looking forward for your feedback!

István Csanády's picture

Amazing job, I have been waiting for these for a long long time. Congrats! This is definitely a huge step forward for OCCT.

Heinrich Kiehm's picture

Viewer is Working now I included the environment variable CSF_GRAPHICSHR = e:\cas3.0\windows_NT\dll\opengl.dll

Thank you for quick answer

Heinrich Kiehm

Pawel's picture

I have read this article very carefully and I'm very impressed by the amount of changes that have been made. Congratulations!

However, there's one point I'd like to comment on. And that would be BOPs again ;)

There are a couple of cases where I still use the old BOPs. One is documented as Issue #24417. (new BOPs do not recognize a BSpline plane as a plane and don't want to proceed this case).

Besides, the old BOPs is the only option for me in the following case:

Result_1 = CreateEdgeAsSectionBetweenFaceAndShape (new BOP)
CreateRectangularFace
Result_2 = SectionBetweenTheRectangularFace And Result_1 (old BOP)
GetCoordinatesOf Result_2 //Result_2 is a point

I'm not sure about the reasons but new BOPs do not work in second case.

There might be some other cases where old BOPs might still be needed that's why I would opt for leaving them in OCCT for a while...

Pawel

Kirill Gavrilov's picture

There might be some other cases where old BOPs might still be needed that's why I would opt for leaving them in OCCT for a while...
the instability of Boolean operations is one of the reasons for keeping different implementations - if first doesn't work you can try another one... Nice cheat for experienced users which are familiar with specific bugs in BOPs.

However this practice is broken - different implementations bloat the code and API confusing the end-user. Anyway, user would expect OCCT to provide single implementation which would be fast, robust and maintainable at once - for all use cases. So fixing known critical bugs is one of the key points on this way. And old unmaintained code should finally gone...

Pawel's picture

Corresponding issue registered.

Mikhail Sazonov's picture

Issue was answered. It seems all works, but it is needed to study how to use new BOP.

Andrey BETENEV's picture

Hello Pawel,

Thank you for commenting!

Naturally old BOP can be removed only when we are sure that main BOP algorithm is more robust and functional than old one, and can produce good result in all valid cases handled successfully by the old algo. It was exactly the purpose of my message to get comments of community users on that. We have taken a note on the issue you have reported, and will analyze it and other issues of this kind before taking the decision. If you know other cases when main BOP behave worse than old one, please report them also, so we could take them into consideration as well.

Andrey

Roman Lygin's picture

Hi Andrey,

Thank you very much for pre-announcement and giving significant lead time to provide feedback on upcoming changes.

First off, amount of made changes and invested efforts seems huge. House-keeping and code clean up often takes a lot of time and results are not always user-visible. So kudos and congratulations to you and your entire team for dedication during all these years.

Thank you also for (temporary) keeping WOK support. At least with that I could quickly rebuild the git sources in a usual manner (I remain conservative here, although many in our team already prefer cmake).

We have not proceeded to porting yet, so these are very preliminarily thoughts. More feedback will come during the course.
1. SFINAE issue in Standard_Handle.hxx. To address the compiler support of default template arguments in functions, have you considered the following approach?:

namespace detail {
template<typename T2, typename T, typename = typename std::enable_if<std::is_base_of<T2, T>::value >
struct handle_cast_helper
{
static const handle<T2>& cast (const T& theObject) const
{ return reinterpret_cast<const handle<T2>&> (theObject); }

static handle<T2>& cast (T& theObject) const
{ return reinterpret_cast< handle<T2>&> (theObject); }
};
}
...

//! Upcast to const reference to base type.
template <class T2>
operator const handle<T2>& () const
{
return detail::handle_cast_helper <T2, T>::cast (*this);
}

2. To be or not to be for bool cast in handle.

Please do decide in favor of /*explicit*/ operator bool(), just like intrusive_ptr<>, shared_ptr<> and other smart pointers. The use case is very common:

template<typename T>
static void foo (const T& theObject)
{
if (theObject) {
theObject->DoSomething();
}
}

MyClass* p = ...;
shared_ptr<MyClass> sp = ...;
handle<MyClass> h = ...;
foo (p);
foo (sp);
foo (h);

This will allow to use easy drop-in replacement, without a need for extra helper policy:
if (is_null_helper<T>() (theObject)) {
theObject->DoSomething();
}

Another strong argument is preserving source code compatibility with legacy code. With efforts required to port, preserving this source compatibility will be at least a minor relief.

Thank you for your considerations. To be continued.
Roman

Kirill Gavrilov's picture

Hello Roman,

thank you for feedback.

2. To be or not to be for bool cast in handle.

Please do decide in favor of /*explicit*/ operator bool()
...
Another strong argument is preserving source code compatibility with legacy code

what do you mean by compatibility with legacy code?
OCCT handle became formally compatible with such kind of implicit casts only from 6.8.0 release.

Concerning explicit bool operator - the main issue is missing support from msvc, which would require very tricky code to make it work.

Roman Lygin's picture

Hello Kirill,

Thank you for your fast response.

Indeed, I meant the user's code which could be written as of 6.8.0. As given in the motivation of #24533 this sort of code (if (h) { ... } ) is typical to be written, especially by novice users.

Other motivations in favor of having bool operator are:
a. better compliance with std/boost
b. currently (until bool operator is declared private) a compiler would still generate a cast, so having an explicitly defined would rather be preferred.

As far as MSVC is concerned you could define a macro (similar to Standard_OVERRIDE) for a bool operator. It could unroll into explicit bool operator for C++11 compliant compilers and into something simpler (such as bool operator) or more complex (what Boost does to implement safe bool idiom) for others.

Hope this helps.
Roman

Andrey BETENEV's picture

Hello Roman,

This is remark on your point 1. "SFINAE issue"; other points are commented below.

In my understanding, your proposed change will not help: the cause of the issue is that cast operator is defined for any type T2, this is the reason of ambiguity. Your modification does not change that. The only way to use SFINAE here is to use default template argument of template functions; alas, MSVC 10 and 11 do not support it.

Note that Boost documentation says:

There does not seem to be a way to specify an enabler for a conversion operator. Converting constructors, however, can have enablers as extra default arguments.

I suppose that this phrase has been written before default template arguments were allowed in template methods.

Andrey

Roman Lygin's picture

Hello again Andrey,

3. 7.0 Alpha drops hierarchy of handles. Instead handle to derived class can be upcasted to a handle to a base class.

AFAIU, handle’s upcast (implemented via reinterpret_cast) violates strict aliasing rule. If so, then this may lead to a UB (undefined behavior) zone and the code can behave arbitrarily depending on compiler options chosen in user’s environment.

Unlike shared_ptr which has a template constructor from shared_ptr to derived type and thus introduces performance penalty, an explicit hierarchy would still allow to gracefully use handles in containers. For instance, let's compare:

std::vector<shared_ptr<Geom_Curve> > v1;
v1.push_back (shared_ptr<Geom_Line> (new Geom_Line (...) )); //template ctor will be called – performance penalty

std::vector<handle<Geom_Curve> > v2;
v2.push_back (handle<Geom_Line> (new Geom_Line (...) )); //upcast will be called in 7.0alpha. UB ?!

If handle<Geom_Line> inherited handle<Geom_Curve> then no upcasts would be called and no performance penalty would take place.

If you considered and refused that idea what were your considerations ?

Thank you.

Best regards,
Roman

Roman Lygin's picture

Correction: the above scenarios will demonstrate similar performance as in the latter case, there will still be a copy constructor (called by the std::vector itself).

So, let's consider another scenario instead:

class foo
{
public:
const std::shared_ptr<Geom_Circle>& Section() const;
const handle<Geom_Line>& Axis() const;
};

foo f;
const std::shared_ptr<Geom_Curve>& s = f.Section(); //template ctor called, performance penalty
const handle<Geom_Curve>& a = f.Axis(); //upcast called, UB ?!

Kirill Gavrilov's picture

If handle inherited handle then no upcasts would be called and no performance penalty would take place.

If you considered and refused that idea what were your considerations ?

preserving handles hierarchy was the first idea - it is nice feature of original handles.
However forward declarations of handles across OCCT code everywhere make this way too complicated within templates.

Roman Lygin's picture

Understood, thank you.

Then what is a practical benefit of a new template-based handle mechanism ?

RTTI could be indeed reimplemented using standard capabilities but what is a point to have a template instead of an explicit class? Handles are the corner stone of OCC and their hierarchy has been used throughout the code bases, both OCC and users'.

Now the trade-off has been made towards C++ templates and losing key performance benefits or introducing (unacceptably?) high UB risk. What gives ?

Mikhail Sazonov's picture

Roman, what is UB? I am not a fond of abbreviations.

Roman Lygin's picture

Hi Mikhail,

Please see the discussion above - undefined behavior (UB is a frequently used shortcut for that).

More on strict aliasing with practical examples of UB:
http://cellperformance.beyond3d.com/articles/2006/06/understanding-stric...
http://blog.qt.io/blog/2011/06/10/type-punning-and-strict-aliasing/
http://habrahabr.ru/post/114117/

My concern is that even if OCC itself builds with safe compiler flags, the user's code which will compile user-defined and/or OCC handle instantiations can enforce the strict aliasing rule leading to UB.

Best regards,
Roman

Roman Lygin's picture

One of our teammates (a C++ fan) who just returned from vacation came out with this proposal / example:

struct Base
{
typedef void base_type;
};

struct Derived : Base
{
typedef Base base_type;
};

template <class T>
class handle;

template <>
class handle<void>
{
};

template <class T>
class handle: public handle<typename T::BaseType>
{
};
I did not realize that template class can inherit itself hence did not think that such a trick is feasible.

Given that base_type is injected by DEFINE_STANDARD_RTTI for any handle-manipulated class, looks like a feasible implementation, doesn't it ? Forward declaration should not complicate the stuff here. Am I missing anything ?

Kirill Gavrilov's picture

I don't get what is a special about quoted example - we have tried many alternatives.
But you are welcome to try it yourself and provide the patch if it will work!

Roman Lygin's picture

Kirill:

The patch is as straightforward as given in the example:

diff --git a/src/Standard/Standard_Handle.hxx b/src/Standard/Standard_Handle.hxx
index 5287770..2d2ac74 100644
--- a/src/Standard/Standard_Handle.hxx
+++ b/src/Standard/Standard_Handle.hxx
@@ -24,6 +24,12 @@ class Standard_Transient;

namespace opencascade {

+ template <class T>
+ class handle;
+
+ template<>
+ class handle<void>;
+
//! Intrusive smart pointer for use with Standard_Transient class and its descendants.
//!
//! This class is similar to boost::intrusive_ptr<>, with additional
@@ -32,7 +38,7 @@ namespace opencascade {
//! which allows it to be passed by reference
//! in functions accepring reference to handle to base class.
template <class T>
- class handle
+ class handle : public handle<typename T::base_type>
{
public:
//! STL-compliant typedef of contained type
@@ -175,49 +181,6 @@ namespace opencascade {
return handle (dynamic_cast<T*>(const_cast<T2*>(thePtr)));
}

-#if __cplusplus >= 201103L || (defined(_MSC_VER) && _MSC_VER >= 1800) || (defined(__GNUC__) && __GNUC__ >= 4 && __GNUC_MINOR__ >= 3)
-
- //! Upcast to const reference to base type.
- template <class T2, typename = typename std::enable_if<std::is_base_of<T2, T>::value>::type>
- operator const handle<T2>& () const
- {
- return reinterpret_cast<const handle<T2>&>(*this);
- }
-
- //! Upcast to non-const reference to base type.
- //! NB: this cast can be dangerous, see #26377
- template <class T2, typename = typename std::enable_if<std::is_base_of<T2, T>::value>::type>
- operator handle<T2>& ()
- {
- return reinterpret_cast<handle<T2>&>(*this);
- }
-
-#else /* fallback version for compilers not supporting default arguments of function templates (VC10, VC11, GCC below 4.3) */
-
- //! Upcast to const reference to base type.
- //! NB: this implementation will cause ambiguity errors on calls to overloaded
- //! functions accepting handles to different types, since compatibility is
- //! checked in the cast code rather than ensured by SFINAE (possible with C++11)
- template <class T2>
- operator const handle<T2>& () const
- {
- // error "type is not a member of enable_if" will be generated if T2 is not sub-type of T
- // (handle is being cast to const& to handle of non-base type)
- return reinterpret_cast<typename std::enable_if<std::is_base_of<T2, T>::value, const handle<T2>&>::type>(*this);
- }
-
- //! Upcast to non-const reference to base type.
- //! NB: this cast can be dangerous, see #26377
- template <class T2>
- operator handle<T2>& ()
- {
- // error "type is not a member of enable_if" will be generated if T2 is not sub-type of T
- // (handle is being cast to const& to handle of non-base type)
- return reinterpret_cast<typename std::enable_if<std::is_base_of<T2, T>::value, handle<T2>&>::type>(*this);
- }
-
-#endif
-

Hierarchy is provided, cast operators are disabled. Entire OCC compiles fine. The limited test suite (testgrid blend) passes fine.
Does that help ?

Roman

Roman Lygin's picture

Submitted as #26497

Kirill Gavrilov's picture

Thank you!
We will look at the patch soon.

Andrey BETENEV's picture

Hello Roman,

Thank you for feedback!

Regarding inheriting handles: it was the first approach tried, just following the old OCCT approach. The typedef base_type in DEFINE_STANDARD_RTTI is artifact of this first version.

This approach has serious drawback - dependency of the handle class on the full hierarchy of the handles of the parents of its argument class.

The main trouble is the need to include header defining handle of the parent class. In OCCT 6.x WOK generated separate header files defining handle class for each transient class defined in CDL, and the same approach was used in most non-CDL classes. In OCCT 7.0 we wanted to remove these trivial headers (they are thousands) to reduce the amount of the code being maintained. Unfortunately, attempt to move definitions of handle classes to headers where the corresponding classes are defined leads to cyclic dependencies between dozens of headers in OCCT. Elimination of these cycles is nontrivial and requires a lot of effort, thus we stopped at that point.

The need to specify a parent when defining a handle for a class is a trouble in its own. In OCCT 6.x this is done by second argument of a macro DEFINE_STANDARD_HANDLE, and use of that macro is a necessity. In version 7.0 we wanted to avoid this necessity. One possibility to avoid use of this macro is to require the argument class to define its base class as typedef with predefined name (base_type), and use that typedef to define a base class for a handle. This has been tried successfully, however causes the same problem of cyclic dependencies between headers as described above.

Note that the patch proposed in branch CR26497 is very similar to that initial version (even if simplified) and suffers from the same problem. Please cross-check whether you actually has been able to compile this version. I guess you might have changed the header only in src directory, not updating it in inc, and hence effectively compiling without the change.

Current implementation is closer to STL shared_ptr: handles do not inherit each other, and this makes their use much simpler. It is sufficient to have forward declaration of a class to be able to use handle to it in declarations, and the only requirement to the argument class is: to be descendant of Standard_Transient.

One more problem of inheriting handles is the one you have indicated when discussing non-CDL version of OCCT last year: handle can be passed as argument to function accepting non-const reference to handle to its base class, where it can be assigned to incompatible type. Though new handles in current master allow this cast for compatibility, in the future we can eliminate this potentially dangerous feature. See issue #26377 for that.

As for the potential issue with breakage of strict aliasing rules, I have doubts if it is a real problem. In my practice I do not remember any single case when violation of strict aliasing rules caused program error. As far as I know, it is only GCC that has this issue, and it outputs warning where it can potentially occur. We have seen these warnings where C-style casts were used for casting handles to derived types, which is dangerous by itself. These casts shall be replaced by calls to DynamicCast(). After that change we have seen no GCC warnings any more.

Actually it is difficult for me to imagine a code that would suffer from strict aliasing problem in case of handles. I will be glad to consider and check your sample if you can provide it.

Note that we do not consider current implementation as final. Something will definitely be improved, in particular it is already clear (also from your another comment) that operator of cast to bool is necessary. We even do not abandon completely the possibility to change implementation to restore inheritance of handles (it should be even easier to do that on current state of code). However, we need more definite arguments to consider this change.

I shall be glad to see more comments from you and other people.

Andrey

Sebastian Hoogen's picture

Thank you for your Amazing work.

I tried to compile our project against the 7.0 alpha. But I still did not manage to modify SMESH accordingly. At this time I'm struggling with SMESH_MeshVSLink.

Did somebody try this, as well?

Andrey BETENEV's picture

Hello Sebastian,

Please wait a couple of weeks -- we are finalizing upgrade tool, which should make upgrade of dependent code mostly automatic. Besides, SALOME is one of projects used for tuning upgrade process.

If you are still going to move on yourself and detect some issues, you can describe them here (with reference to code and problem you face) -- we shall try to give some advises basing on our experience.

Andrey

Andrey BETENEV's picture

Hello,

The procedure of upgrading application code to adapt to changes described above is ready for review and testing. See OCCT Git master branch and Upgrade Guide.
Note that OCCT code has also been corrected to improve compatibility with code based on OCCT 6.x; in particular, operator of cast of Handle to bool is provided, thus Handles can be used in conditional expressions directly.

Your remarks and suggestions for possible improvements are welcome.
If you apply the procedure to your code, please comment here to share your experience.

Andrey

Roman Lygin's picture

Hi Andrey,

Mantis (http://tracker.dev.opencascade.org/roadmap_page.php) says that 7.0 is scheduled to Nov 30, 2015. Is this Beta or Production ?

We just started migration and after 3 working days maybe are only at ~20% of the path. Broken source compatibility (due to handles rewrite) is the most significant factor contributing to slow pace.
Given that there are a few severe issues (e.g. #26913, #26914) would you mind having at least one Beta release prior to Production ?

Migration can really take significant efforts (primarily due to handles rewrite) and perhaps many users just did not start doing that. You might probably be interested in some practical feedback before closing the gate.

Thank you for your consideration.
Roman

Andrey BETENEV's picture

Hello Roman,

I confirm that we have to delay OCCT 7.0 release. The reason of delay are a few important changes that should get into OCCT 7.0 but found not sufficiently good at certification. It will take 1-2 weeks more to finalize them.

Thus currently we aim to release beta version by middle of December. Then, we will have about one month for testing before the final release.

Andrey

Roman Lygin's picture

Thanks for the update Andrey.

On our side we have been also progressing porting to 7.0 API. It took about 7-10 person-days in total which have been the greatest effort so far. The mainstream test suite revealed a few issues which been reported via Mantis.
GUI is hopefully to be finalized soon.

Performance-wise there are a few substantial regressions - about 30% tests (150 of 520) reveal 10%-1200% slow down. The main suspicion is changed B-Spline cache management. So this has to be investigated further but I am afraid this will largely have to be put off as we run out of available cycles to spend on this porting effort.

Will keep you posted.

Thank you again.
Roman

Andrey BETENEV's picture

Hello,

Please note that integration raised last Friday to the master branch of the OCCT Git repository contains change in the macros used for defining OCCT RTTI. Instead of single macro DEFINE_STANDARD_RTTI, two other macros are provided:

- DEFINE_STANDARD_RTTI_INLINE - defines all methods required for OCCT RTTI (DynamicType()) as inline, and can be used just in the same way as macro DEFINE_STANDARD_RTTI before that change.

- DEFINE_STANDARD_RTTIEXT - defines RTTI methods as out-of-line, and requires that they were instantiated in source by macro IMPLEMENT_STANDARD_RTTIEXT. The latter macro is just the same as used in all previous versions of OCCT.

The upgrade procedure can be used to update the code based on previous state of the master, to be consistent with the current version:

> upgrade.bat -rtti -src=path_to_your_sources -recurse

It will replace DEFINE_STANDARD_RTTI with two arguments by one of the above macros. If this macro is found in header, and there is source file with the same name in the same folder, it will use DEFINE_STANDARD_RTTIEXT, and restore IMPLEMENT_STANDARD_RTTI in source if it was not present. Otherwise, DEFINE_STANDARD_RTTI_INLINE will be used.

The primary goal of this change is to avoid bloating of the size of binaries of OCCT itself and of depending projects; see http://dev.opencascade.org/index.php?q=node/1137 for some details. As well, this change should eliminate some extra dependencies between libraries, and facilitate writing code that is compatible with both OCCT 6.x and 7.x (due to use of different names for macros with different arguments).

Andrey