View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0026460CommunityOCCT:Modeling Algorithmspublic2015-07-19 13:192016-04-20 15:48
Reportercshorler Assigned Toski 
PrioritynormalSeveritytweak 
Status closedResolutionfixed 
PlatformLinux x86_64OSopenSUSE (x86_64) 
Product Version6.9.0 
Target Version7.0.0Fixed in Version7.0.0 
Summary0026460: Implicit cast to TopoDS_Shape compilation error due to ambiguous conversion
DescriptionCompiler: 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;
                 ^
Steps To Reproducetest.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

TagsNo tags attached.
Test case numberNot needed

Attached Files

  • test.cpp (170 bytes)

Activities

cshorler

2015-07-19 13:19

developer  

test.cpp (170 bytes)

git

2015-07-19 13:28

administrator   ~0043172

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

msv

2015-07-20 11:03

developer   ~0043175

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.

cshorler

2015-07-21 01:16

developer   ~0043201

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.

msv

2015-07-21 09:49

developer   ~0043203

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.

cshorler

2015-07-22 01:10

developer   ~0043249

ok, understood.

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

Is a patch still needed?

msv

2015-07-22 09:23

developer   ~0043270

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.

cshorler

2015-07-28 10:27

reporter   ~0043610

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(-)

msv

2015-07-28 11:56

developer   ~0043619

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.

git

2015-08-01 21:09

administrator   ~0043772

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

cshorler

2015-08-01 21:14

reporter   ~0043773

Please note the git branch is now CR26460_2

git

2015-08-02 14:31

administrator   ~0043774

Branch CR26460_2 has been updated forcibly by cshorler.

SHA-1: 499a2cb89dc38ff3f7c6e08899ea7ef1f6e7e407

msv

2015-08-03 11:44

developer   ~0043779

Reviewed.

mkv

2015-08-04 17:45

tester   ~0043819

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.

git

2015-08-14 10:52

administrator   ~0044132

Branch CR26460_2 has been deleted by inv.

SHA-1: 499a2cb89dc38ff3f7c6e08899ea7ef1f6e7e407

git

2015-08-14 10:55

administrator   ~0044179

Branch CR26460 has been deleted by inv.

SHA-1: 644a5844896f9e86c2393291da40035e1a24efbf

Related Changesets

occt: master ecac41a9

2015-08-13 07:52:52

cshorler


Committer: ski Details Diff		 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

fix compilation for CR26460
- make argument for IsWRCase2 (non-const) BRepAlgoAPI_BooleanOperation&		 Affected Issues
0026460
mod - src/BRepAlgoAPI/BRepAlgoAPI_Algo.cxx Diff File
mod - src/BRepAlgoAPI/BRepAlgoAPI_Algo.hxx Diff File
mod - src/BRepBuilderAPI/BRepBuilderAPI_MakeEdge.cxx Diff File
mod - src/BRepBuilderAPI/BRepBuilderAPI_MakeEdge.hxx Diff File
mod - src/BRepBuilderAPI/BRepBuilderAPI_MakeEdge2d.cxx Diff File
mod - src/BRepBuilderAPI/BRepBuilderAPI_MakeEdge2d.hxx Diff File
mod - src/BRepBuilderAPI/BRepBuilderAPI_MakePolygon.cxx Diff File
mod - src/BRepBuilderAPI/BRepBuilderAPI_MakePolygon.hxx Diff File
mod - src/BRepBuilderAPI/BRepBuilderAPI_MakeShape.cxx Diff File
mod - src/BRepBuilderAPI/BRepBuilderAPI_MakeShape.hxx Diff File
mod - src/BRepBuilderAPI/BRepBuilderAPI_MakeSolid.cxx Diff File
mod - src/BRepBuilderAPI/BRepBuilderAPI_MakeSolid.hxx Diff File
mod - src/BRepBuilderAPI/BRepBuilderAPI_MakeVertex.cxx Diff File
mod - src/BRepBuilderAPI/BRepBuilderAPI_MakeVertex.hxx Diff File
mod - src/BRepBuilderAPI/BRepBuilderAPI_MakeWire.cxx Diff File
mod - src/BRepBuilderAPI/BRepBuilderAPI_MakeWire.hxx Diff File
mod - src/BRepLib/BRepLib_MakeEdge.cxx Diff File
mod - src/BRepLib/BRepLib_MakeEdge.hxx Diff File
mod - src/BRepLib/BRepLib_MakeEdge2d.cxx Diff File
mod - src/BRepLib/BRepLib_MakeEdge2d.hxx Diff File
mod - src/BRepLib/BRepLib_MakePolygon.cxx Diff File
mod - src/BRepLib/BRepLib_MakePolygon.hxx Diff File
mod - src/BRepLib/BRepLib_MakeShape.cxx Diff File
mod - src/BRepLib/BRepLib_MakeShape.hxx Diff File
mod - src/BRepLib/BRepLib_MakeSolid.cxx Diff File
mod - src/BRepLib/BRepLib_MakeSolid.hxx Diff File
mod - src/BRepLib/BRepLib_MakeVertex.cxx Diff File
mod - src/BRepLib/BRepLib_MakeVertex.hxx Diff File
mod - src/BRepLib/BRepLib_MakeWire.cxx Diff File
mod - src/BRepLib/BRepLib_MakeWire.hxx Diff File
mod - src/QANewBRepNaming/QANewBRepNaming_BooleanOperationFeat.cxx Diff File
mod - src/QANewBRepNaming/QANewBRepNaming_BooleanOperationFeat.hxx Diff File

Issue History

Date Modified Username Field Change
2015-07-19 13:19 cshorler New Issue
2015-07-19 13:19 cshorler Assigned To => msv
2015-07-19 13:19 cshorler File Added: test.cpp
2015-07-19 13:28 git Note Added: 0043172
2015-07-19 13:32 cshorler Status new => resolved
2015-07-19 19:36 cshorler Steps to Reproduce Updated
2015-07-20 11:03 msv Note Added: 0043175
2015-07-20 11:05 msv Assigned To msv => myn
2015-07-20 11:05 msv Status resolved => assigned
2015-07-21 01:16 cshorler Note Added: 0043201
2015-07-21 09:49 msv Note Added: 0043203
2015-07-22 01:10 cshorler Note Added: 0043249
2015-07-22 09:23 msv Note Added: 0043270
2015-07-22 09:23 msv Assigned To myn => cshorler
2015-07-28 10:27 cshorler Note Added: 0043610
2015-07-28 11:56 msv Note Added: 0043619
2015-08-01 21:09 git Note Added: 0043772
2015-08-01 21:13 cshorler Assigned To cshorler => msv
2015-08-01 21:13 cshorler Status assigned => resolved
2015-08-01 21:14 cshorler Note Added: 0043773
2015-08-02 14:31 git Note Added: 0043774
2015-08-03 11:44 msv Note Added: 0043779
2015-08-03 11:44 msv Assigned To msv => bugmaster
2015-08-03 11:44 msv Status resolved => reviewed
2015-08-03 13:04 mkv Assigned To bugmaster => mkv
2015-08-04 17:45 mkv Note Added: 0043819
2015-08-04 17:45 mkv Assigned To mkv => bugmaster
2015-08-04 17:45 mkv Status reviewed => tested
2015-08-04 17:46 mkv Test case number => Not needed
2015-08-14 10:26 ski Changeset attached => occt master ecac41a9
2015-08-14 10:26 ski Assigned To bugmaster => ski
2015-08-14 10:26 ski Status tested => verified
2015-08-14 10:26 ski Resolution open => fixed
2015-08-14 10:44 bugmaster Target Version 6.9.0 => 7.0.0
2015-08-14 10:52 git Note Added: 0044132
2015-08-14 10:55 git Note Added: 0044179
2016-04-20 15:44 aiv Fixed in Version => 7.0.0
2016-04-20 15:48 aiv Status verified => closed