MantisBT - Community
View Issue Details
0026460Community[OCCT] OCCT:Modeling Algorithmspublic2015-07-19 13:192016-04-20 15:48
cshorler 
ski 
normaltweak 
closedfixed 
Linux x86_64openSUSE (x86_64)13.2
[OCCT] 6.9.0 
[OCCT] 7.0.0[OCCT] 7.0.0 
Not needed
0026460: Implicit cast to TopoDS_Shape compilation error due to ambiguous conversion
Compiler: g++ -> gcc version 4.8.3

impacted / tested:
OCCT 6.8.0
OCCT 6.9.0
OCCT git HEAD

I believe the error is due to the const member function being an inexact match for the rvalue BRepPrimAPI_MakeBox(...). Adding a non-const version of the cast operator resolves the ambiguity.

The example is for BRepPrimAPI_MakeBox, also tested ..._MakeTorus and a few others with similar effects.

I have tested a patch - will attempt to push to git.


abuild@linux-foxtrot.site:/home/abuild/rpmbuild/BUILD/occt-V6_9_0-133-g644a584/build> g++ -Iinc -c -std=gnu++11 -o test.o test.cpp
test.cpp: In function 'int main(int, char**)':
test.cpp:6:54: error: conversion from 'BRepPrimAPI_MakeBox' to 'TopoDS_Shape' is ambiguous
     TopoDS_Shape S1 = BRepPrimAPI_MakeBox(20.,30.,40.);
                                                      ^
test.cpp:6:54: note: candidates are:
In file included from inc/BRepPrimAPI_MakeBox.hxx:1:0,
                 from test.cpp:2:
/home/abuild/rpmbuild/BUILD/occt-V6_9_0-133-g644a584/src/BRepPrimAPI/BRepPrimAPI_MakeBox.hxx:87:17: note: BRepPrimAPI_MakeBox::operator TopoDS_Solid()
 Standard_EXPORT operator TopoDS_Solid();
                 ^
/home/abuild/rpmbuild/BUILD/occt-V6_9_0-133-g644a584/src/BRepPrimAPI/BRepPrimAPI_MakeBox.hxx:83:17: note: BRepPrimAPI_MakeBox::operator TopoDS_Shell()
 Standard_EXPORT operator TopoDS_Shell();
                 ^
In file included from inc/BRepBuilderAPI_MakeShape.hxx:1:0,
                 from test.cpp:1:
/home/abuild/rpmbuild/BUILD/occt-V6_9_0-133-g644a584/src/BRepBuilderAPI/BRepBuilderAPI_MakeShape.hxx:54:17: note: BRepBuilderAPI_MakeShape::operator TopoDS_Shape() const
 Standard_EXPORT operator TopoDS_Shape() const;
                 ^
test.cpp attached, amend the include path to g++ as necessary

$ g++ -Iinc -c -std=c++0x -o test.o test.cpp

or (both tested)

$ g++ -Iinc -c -std=gnu++11 -o test.o test.cpp

No tags attached.
cpp test.cpp (170) 2015-07-19 13:19
https://tracker.dev.opencascade.org/
Issue History
2015-07-19 13:19cshorlerNew Issue
2015-07-19 13:19cshorlerAssigned To => msv
2015-07-19 13:19cshorlerFile Added: test.cpp
2015-07-19 13:28gitNote Added: 0043172
2015-07-19 13:32cshorlerStatusnew => resolved
2015-07-19 19:36cshorlerSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=11031#r11031
2015-07-20 11:03msvNote Added: 0043175
2015-07-20 11:05msvAssigned Tomsv => myn
2015-07-20 11:05msvStatusresolved => assigned
2015-07-21 01:16cshorlerNote Added: 0043201
2015-07-21 09:49msvNote Added: 0043203
2015-07-22 01:10cshorlerNote Added: 0043249
2015-07-22 09:23msvNote Added: 0043270
2015-07-22 09:23msvAssigned Tomyn => cshorler
2015-07-28 10:27cshorlerNote Added: 0043610
2015-07-28 11:56msvNote Added: 0043619
2015-08-01 21:09gitNote Added: 0043772
2015-08-01 21:13cshorlerAssigned Tocshorler => msv
2015-08-01 21:13cshorlerStatusassigned => resolved
2015-08-01 21:14cshorlerNote Added: 0043773
2015-08-02 14:31gitNote Added: 0043774
2015-08-03 11:44msvNote Added: 0043779
2015-08-03 11:44msvAssigned Tomsv => bugmaster
2015-08-03 11:44msvStatusresolved => reviewed
2015-08-03 13:04mkvAssigned Tobugmaster => mkv
2015-08-04 17:45mkvNote Added: 0043819
2015-08-04 17:45mkvAssigned Tomkv => bugmaster
2015-08-04 17:45mkvStatusreviewed => tested
2015-08-04 17:46mkvTest case number => Not needed
2015-08-14 10:26skiChangeset attached => occt master ecac41a9
2015-08-14 10:26skiAssigned Tobugmaster => ski
2015-08-14 10:26skiStatustested => verified
2015-08-14 10:26skiResolutionopen => fixed
2015-08-14 10:44bugmasterTarget Version6.9.0 => 7.0.0
2015-08-14 10:52gitNote Added: 0044132
2015-08-14 10:55gitNote Added: 0044179
2016-04-20 15:44aivFixed in Version => 7.0.0
2016-04-20 15:48aivStatusverified => closed

Notes
(0043172)
git   
2015-07-19 13:28   
Branch CR26460 has been created by cshorler.

SHA-1: 644a5844896f9e86c2393291da40035e1a24efbf


Detailed log of new commits:

Author: Christopher Horler
Date: Sat Jul 18 14:06:37 2015 +0100

    When casting from BRepPrimAPI_MakeBox / BRepPrimAPI_MakeTorus / ... classes to TopoDS_Shape, a compilation error happens due to operator ambiguity as there's no direct signature match. Adding a non-const member cast operator to BRepBuilderAPI_MakeShape resolves the issue
(0043175)
msv   
2015-07-20 11:03   
The methods "operator TopoDS_Shape" and "Shape" in BRepBuilderAPI_MakeShape are declared as const, but indeed they are not, because they call Build(), so these methods should not be declared const.
I think API must be honor and call the things by their own names.
So, I propose to make the above mentioned methods non-const. This resolves the problem of compilation with the attached sample.
We shall see how this change will impact compilation of the whole OCCT.
Also, it is needed to check the declarations of similar methods "operator Shell" etc. in other descendants of BRepBuilderAPI_MakeShape, in order to eliminate any contradictions.
(0043201)
cshorler   
2015-07-21 01:16   
This link is interesting in discussing logical const-ness
https://isocpp.org/wiki/faq/const-correctness#logical-vs-physical-state [^]
and this one for practical application (same page)
https://isocpp.org/wiki/faq/const-correctness#logical-vs-physical-const [^]

and noting that IsDone() is public, but Done() is protected, and the final state will likely depend upon the specifics of each use. My knowledge on uses is limited - I saw two patterns when grepping.

BRepBuilderObj builder(...)
builder.AddEdge(..)
builder.AddEdge(...)
TopoDS_Shape S0 = builder.Shape();

and the direct evaluation shown in the example

I wasn't sure if there was a case for using a const builder passed to a function as a generator

i.e.

void
f(const BRepBuilder...& builder) {
   TopoDS S0 = builder.Shape();
   TopoDS S1 = builder.Shape();
   ....
   // do other stuff like apply position
}

I figured there could be more than one use case, this is why I didn't delete the const and added a non-const implicit cast.

Although, I did wonder why return by value on the operator when instead the copy constructor call could be eliminated by returning by const reference.
(0043203)
msv   
2015-07-21 09:49   
The method IsDone() is an indicator of the logical state of the builder. The method Shape() calls Done(), thus changing the logical state. So, the method Shape() is actually mutator of logical state, and according to the article linked above it must be declared non-const.

I understand the fear of passing the builder as const reference in a method that will take result from it. But in this case I would prefer to pass non-const reference of the builder, or on contrary call the method Build() before passing const reference to another method.

The things become simpler when objects behave logically. Now, the user sees the method Shape() as const and he might be confused when he realized that this method initiated the actual long-time computation. This confusing will not appear if the user sees that the method is declared non-const, so computation in it will be not surprising anymore.
(0043249)
cshorler   
2015-07-22 01:10   
ok, understood.

I started to look at the classes in the Doxygen inheritance diagram for BRepBuilder_MakeShape.

Is a patch still needed?
(0043270)
msv   
2015-07-22 09:23   
Dear Christopher,
We shall appreciate your contribution. If you are willing to lead this development to the end, then please prepare the complete patch taking into account considerations of my comment 0026460:0043175. When ready, change status to "resolved" and assign to me. I shall review and pass to certification stage.
(0043610)
cshorler   
2015-07-28 10:27   
I noticed that two methods e.g. BRepPrimAPI_MakeHalfSpace::Solid() raise an exception rather than call Build(). Should this behaviour be consistent everywhere?

I'm nearly done (I still need to do a build test before committing the new branch)

FYI currently impacted files:
 src/BRepBuilderAPI/BRepBuilderAPI_MakeEdge.cxx | 4 ++--
 src/BRepBuilderAPI/BRepBuilderAPI_MakeEdge.hxx | 4 ++--
 src/BRepBuilderAPI/BRepBuilderAPI_MakeEdge2d.cxx | 4 ++--
 src/BRepBuilderAPI/BRepBuilderAPI_MakeEdge2d.hxx | 4 ++--
 src/BRepBuilderAPI/BRepBuilderAPI_MakePolygon.cxx | 4 ++--
 src/BRepBuilderAPI/BRepBuilderAPI_MakePolygon.hxx | 4 ++--
 src/BRepBuilderAPI/BRepBuilderAPI_MakeShape.cxx | 4 ++--
 src/BRepBuilderAPI/BRepBuilderAPI_MakeShape.hxx | 4 ++--
 src/BRepBuilderAPI/BRepBuilderAPI_MakeSolid.cxx | 4 ++--
 src/BRepBuilderAPI/BRepBuilderAPI_MakeSolid.hxx | 4 ++--
 src/BRepBuilderAPI/BRepBuilderAPI_MakeVertex.cxx | 4 ++--
 src/BRepBuilderAPI/BRepBuilderAPI_MakeVertex.hxx | 4 ++--
 src/BRepBuilderAPI/BRepBuilderAPI_MakeWire.cxx | 4 ++--
 src/BRepBuilderAPI/BRepBuilderAPI_MakeWire.hxx | 4 ++--
 src/BRepLib/BRepLib_MakeEdge2d.cxx | 4 ++--
 src/BRepLib/BRepLib_MakeEdge2d.hxx | 4 ++--
 src/BRepLib/BRepLib_MakePolygon.cxx | 4 ++--
 src/BRepLib/BRepLib_MakePolygon.hxx | 4 ++--
 src/BRepLib/BRepLib_MakeShape.cxx | 4 ++--
 src/BRepLib/BRepLib_MakeShape.hxx | 4 ++--
 src/BRepLib/BRepLib_MakeSolid.cxx | 4 ++--
 src/BRepLib/BRepLib_MakeSolid.hxx | 4 ++--
 src/BRepLib/BRepLib_MakeVertex.cxx | 4 ++--
 src/BRepLib/BRepLib_MakeVertex.hxx | 4 ++--
 src/BRepLib/BRepLib_MakeWire.cxx | 4 ++--
 src/BRepLib/BRepLib_MakeWire.hxx | 4 ++--
 26 files changed, 52 insertions(+), 52 deletions(-)
(0043619)
msv   
2015-07-28 11:56   
Great work!
BRepPrimAPI_MakeHalfSpace has correct behavior of Solid() method, because it does not have Build() method redefined, and all the job is done in constructor. So, if constructor did not succeed then Solid() can do nothing else but throw exception.
(0043772)
git   
2015-08-01 21:09   
Branch CR26460_2 has been created by cshorler.

SHA-1: dad140d2e9acb8f73dfd42f3efa844beda9aef43


Detailed log of new commits:

Author: Christopher Horler
Date: Sat Aug 1 19:00:21 2015 +0100

    fix compilation for CR26460
    - make argument for IsWRCase2 (non-const) BRepAlgoAPI_BooleanOperation&

Author: Christopher Horler
Date: Sat Aug 1 18:54:42 2015 +0100

    0026460: Implicit cast to TopoDS_Shape compilation error due to ambiguous conversion
    
    - make operator TopoDS_Shape() cast a "non-const" member function
    - make operator TopoDS_*() cast "non-const" when mutating behaviour is possible
    - align derived class methods and overrides with the above changes
(0043773)
cshorler   
2015-08-01 21:14   
Please note the git branch is now CR26460_2
(0043774)
git   
2015-08-02 14:31   
Branch CR26460_2 has been updated forcibly by cshorler.

SHA-1: 499a2cb89dc38ff3f7c6e08899ea7ef1f6e7e407
(0043779)
msv   
2015-08-03 11:44   
Reviewed.
(0043819)
mkv   
2015-08-04 17:45   
Dear BugMaster,
Branch CR26460_2 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested on Release mode.
SHA-1: 499a2cb89dc38ff3f7c6e08899ea7ef1f6e7e407

Number of compiler warnings:

occt component :
Linux: 15 (15 on master)
Windows: 0 (0 on master)

products component :
Linux: 39 (39 on master)
Windows: 0 (0 on master)

Regressions/Differences/Improvements:
No regressions/differences

Testing cases:
Not needed

Testing on Linux:
occt component :
Total MEMORY difference: 91635647 / 91602443 [+0.04%]
Total CPU difference: 17493.989999999394 / 18131.539999999095 [-3.52%]
products component :
Total MEMORY difference: 24845317 / 24833993 [+0.05%]
Total CPU difference: 6954.510000000003 / 6149.739999999959 [+13.09%]

Testing on Windows:
occt component :
Total MEMORY difference: 57610838 / 57632139 [-0.04%]
Total CPU difference: 16209.892308898981 / 16351.806418599175 [-0.87%]
products component :
Total MEMORY difference: 16011806 / 16018438 [-0.04%]
Total CPU difference: 5176.815184499968 / 5283.23906669996 [-2.01%]

There are no differences in images found by testdiff.
(0044132)
git   
2015-08-14 10:52   
Branch CR26460_2 has been deleted by inv.

SHA-1: 499a2cb89dc38ff3f7c6e08899ea7ef1f6e7e407
(0044179)
git   
2015-08-14 10:55   
Branch CR26460 has been deleted by inv.

SHA-1: 644a5844896f9e86c2393291da40035e1a24efbf