Thu, 05/05/2011 - 02:50
Forums:
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.
Attachments:
Collaborative development portal
Thu, 05/05/2011 - 08: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
Thu, 05/05/2011 - 19: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.
Thu, 05/05/2011 - 22: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
Thu, 05/05/2011 - 22: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.
Thu, 05/05/2011 - 23: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.
Fri, 05/06/2011 - 00: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;...
I guess that many people will be really happy if OCC uses proper library versioning.
Fri, 05/06/2011 - 10: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
Fri, 05/06/2011 - 10:30
Oops you are right, thanks for the catch
Fri, 05/06/2011 - 10: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.
Fri, 05/06/2011 - 10:58
Hello Roman,
You miss the point, Value() is for compatibility only, I do not care whether it is slower than SquareDistance().
Fri, 05/06/2011 - 13:21
Done, patch is now fixed, thanks for your comments.