Open CASCADE, the 3D modelling kernel
3D modeling & numerical simulation

Search the Forums
See All Topics
Open CASCADEShowroomGet it!Developer CornerSupport and ProductsAbout us
Technical overview
Areas of use
Advantages
FAQ
Screenshots
Shape factory
Shape gallery
Demonstrations
What's new
System requirements
Download Center
Public license
Documentation
Getting started
Forums
Open Source community
Training and e-learning
A-la Carte Support
Value-added software
Complementary Components
Customer Corner
Company Profile
Marketing Materials
Contact Us
News
Home / Developer Corner / Forums / Installation and building / [Patch] Fix build failures with g++ 4.6

[Patch] Fix build failures with g++ 4.6

[Patch] Fix build failures with g++ 4.6
Denis Barbier 2011/05/05 00:50
Hello,

Here is a patch to fix build failures with g++ 4.6. It is still error prone, it would be better to not use address of local variables at all.
You have to be logged in to download the attached file
Roman Lygin 2011/05/05 06:59
Hi Denis,

Thanks for the patch. I would recommend solving this problem differently. Instead of creating explicit temporary local variables Visual3d_Layer API could be changed - to return const& instead of a copy of its field. That is:

Visual3d_Layer.cdl:
CLayer (me)
returns CLayer2d from Aspect;
---C+: return const&

In Visual3d_Layer.hxx and Visual3d_Layer.cxx:
const Aspect_CLayer2d& Visual3d_Layer::CLayer () const

In this case, no modifications will be required in its callers (Visual3d_TransientManager.cxx) as taking address will be valid.

Hope this helps.
Roman
Denis Barbier 2011/05/05 17:28
Hello Roman,

That works, but it would be great to not introduce incompatible changes without good reasons. If this prototype is to get fixed, please keep the old one as is (no idea whether WOK can tag it as being deprecated) but add a new method.
Roman Lygin 2011/05/05 20:07
Hi Denis,

Introducing a method with the same name and const qualifier and only differing in returned value (T vs const T&) will cause compilation error.
Your concern about binary compatibility is valid, but OCC policy is usually very relaxed and compatibility often breaks from release to release, so I wouldn't treat that as significant obstacle. The source compatibility will be retained. Of course, modifying the CDL or header .hxx file always adds extra overhead but it is often worth it for the sake of more clean and reliable code.
My 2 cents...
Roman
Denis Barbier 2011/05/05 20:41
Hello Roman,

For the record, I did not advocate for adding a new method with the same name.

It is true that your proposed change does not break source compatibility, which is fine, but it would be nice if OCC could avoid spurious API changes, it is a pain. For instance I decided to patch the Debian package to provide a Value method in classes where it had been renamed into SquareDistance, so that other Debian packages can be rebuild against OCC 6.5.0 without much trouble.
Roman Lygin 2011/05/05 21:15
Hello Denis,

Then I did not get your comment "If this prototype is to get fixed, please keep the old one as is ... but add a new method." If a new method (with a new name) is added returning a const& then this does not address your initial issue of gcc4.6. My point was to change the API and to not change the callers.

As for the Value() vs SquareDistance() - you should blame me as I authored that change ;-). The reason was - as explained here - sqrt() was a bottleneck. To enforce avoiding its usage, the new name replaced the old one. Break of source compatibility allowed to catch all callers during compilation. I would agree that keeping the old syntax (Value()), after all callers have been updated, could be reasonable. Point taken.
Denis Barbier 2011/05/05 22:00
Hello Roman,

Sorry I was unclear. My proposed fix is to add a new method and replace all calls to CLayer() in all OCC code (ie in Visual3d_TransientManager.cxx only) by a call to this new method; then CLayer() would be unused in OCC code, but 3rd party code may contain calls to this method, and since it is declared as public, it would be nice if it was kept for a transitional period.

About the semantic change in Value(), of course it makes perfect sense; but keeping some compatibility layer is quite easy (at least in this case), see
http://git.debian.org/?p=debian-science/packages/opencascade.git;a=blob;f=debian/patches/compatibility-occ630-Value.patch
I guess that many people will be really happy if OCC uses proper library versioning.
Mikhail Sazonov 2011/05/06 08:10
Hi Denis,
I looked at your compatibility patch, and found out that resurrected method Value(n) returns directly SquareDistance(n). That is incorrect. It must return sqrt(SquareDistance(n)), otherwise you get regression in all callers of this method.
Regards,
Mikhail
Denis Barbier 2011/05/06 08:30
Oops you are right, thanks for the catch
Roman Lygin 2011/05/06 08:35
Yes, this is a correct observation, I was going to comment on that too. A few extra comments:

1. Another downside of having Value() returning srqt (SquareDistance()) is that it can be suboptimal - calling sqrt() every time, what may become a bottleneck. An approach of some lazy calculation and storing in the field member:

MyClass::Value()
{
if (myVal < 0.)
myVal = sqrt (SquareDistance);
return myVal;
}
is also impossible given that SqDistances are stored in the sequence containers. And it will also break binary compatibility.

2. These .hxx are auto-generated from CDL by WOK, so modifications should be placed into CDL. Inlined methods will be put into .lxx, so re-generated hxx will override modifications suggested in patch.

3. Standard_EXPORT has no effect on inline methods, so can be omitted. Anyway, WOK will take care of that.

So overall, there is a lot of downsides in the patch 'as is' and making it fully correctly does not really justify it, in my opinion. Though the idea for future does make sense to bear in mind.
My 2 cents.
Denis Barbier 2011/05/06 08:58
Hello Roman,

You miss the point, Value() is for compatibility only, I do not care whether it is slower than SquareDistance().
Denis Barbier 2011/05/06 11:21
Done, patch is now fixed, thanks for your comments.
 
 
Latest news
  • Open CASCADE Technology 6.7.0 is available for download!
  • Open CASCADE Technology 6.6.0 is available for download!
  • Open CASCADE Technology 6.5.5 is available for download!

  • © OPEN CASCADE 2000 - 2014  |  Search  |  Contacts   |  Site map