Archived discussion
Hello Andrey,
This is some early feedback on the 7.0 Upgrade tool.
We are still in the progress of migration, so some comments may come later on.
First off, the tool does help to reduce tedious work and thus speed up migration, so thank you for providing it.
Some comments, not in any particular order:
1. The tool removes extra spaces at the end of lines.
Although not bad per se, this is not its purpose and extra differences only contaminates the diff log. It would be better to disable this by default.
2. Having -compat option is great. However even in compat mode the tool modifies DEFINE_STANDARD_RTTI() to have two arguments (class and baseclass) matching 7.0 conventions but breaking pre-7.0. Would be better to use some new name OCC_DEFINE_STANDARD_RTII(class,base) which would then be defined as single or dual arguments elsewhere. Not a big deal either, we can live with current way and rename the DEFINE_STANDARD_RTTI macro ourselves.
3. Updating forward declarations.
In some case the handle forward declaration is replaced with forward declaration of a class, for instance:
-class Handle_TDocStd_Document;
+class TDocStd_Document;
but sometimes the tool adds inclusion of the header:
-class Handle_TDF_Attribute;
+#include
What triggers different behavior ?
Including a new header file increases compilation times, and we deliberately tried to avoid that using forward declarations instead.
The general approach which would work would be to include and use forward declaration of a class:
#include
class TDF_Attribute;
Hope this will be of any value as a feedback.
Best regards,
Author
Discussion
Posts are displayed in their archived order.
The tool mistakenly decides to insert a header of the kind
#include <Class.hxx>
even if respective header was already included using a sub-directory.
For instance, the original file contained
#include <cadex/ACISGeom_Point.hxx>
The tool added
#include <ACISGeom_Point.hxx>
which breaks compilation and requires manual removal of the just added line.
It is better to parse the include list to look for some regular expression to check if the respective header was already included.
Hello Author,
Thank you for feedback, and sorry for delayed reply..
Let me comment point-by-point:
1. I agree, removal of spaces at ends of lines is not correct, to be fixed.
2. Replacing DEFINE_STANDARD_RTTI by another macro may have sense, also in the view of another your remark, regarding inlined DynamicType().
A possible approach could be to have two macros:
- DEFINE_STANDARD_RTTIEXT -- one just declaring DynamicType(), to be combined with IMPLEMENT_STANDARD_RTTIEXT (as before). This will be default.
- DEFINE_STANDARD_RTTI_INLINE -- just the same as current DEFINE_STANDARD_RTTI in master. This is to be used if class should be completely defined in header.
3. We replace forward declaration of Handle class by #include of its argument in header files that look like implementing QObject classes.
The reason is that when you use handle in signal or slot using syntax "Handle(ClassName)", MOC will fail because it does not recognize macros. Thus to be on the safe side we keep "Handle_ClassName" syntax, and include ClassName.hxx assuming that it defined this typedef (done by DEFINE_STANDARD_HANDLE macro).
4. I agree, upgrade script can be improved to handle this.
I have registered issue #26919 to process points 1 and 4
Best Regards,
Commenter-1
Hi Andrey,
Many thanks for finding time to comment on this and agreement to follow up.
Item 2 would be awesome (given our discussion here and offline during the last few days). Yes, using the old macro IMPLEMENT_STANDARD_RTTIEXT would be just perfect - this would keep source compatibility in .cxx.
And to clarify - please make OCC itself use those two macros (DEFINE & IMPLEMENT). DEFINE_STANDARD_RTTI_INLINE could be used with extra care by OCC but only when the type definition does not cross the module boundaries, e.g. some internal type is used by the library only and in the header when no other OCC library includes.
Looking forward to this upgrade.
Thank you,
Author