MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0029420Community[OCCT] OCCT:Modeling Datapublic2018-01-11 17:242019-08-12 15:15
ReporterBenjaminBihler 
Assigned Toabv 
PrioritynormalSeverityintegration request 
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target VersionFixed in Version 
Summary0029420: Modeling Data - TopoDS_Builder and BRepBuilder should prevent instantiation and their methods should be static
DescriptionTopoDS_Builder and BRepBuilder have only synthesized constructors. They have no state and all their methods are const. That means that they are just collections of methods belonging somehow together. In this case instantiation should be prevented, because it is unnecessary and misleading (the programmer might think that some building state is preserved) and all methods should be static.

A comment in TopoDS_Builder states that the methods are not static to allow redefinition in inherited builders. But no method is redefined anywhere else. If something like the factory design pattern was intended, this won't work in the way these builders are used now, since they are instantiated explicitely. Some indirection was necessary to allow different behaviour of builder child classes.

If you agree, then the next steps should probably be:
- Implement constructors and mark them as deprecated.
- Stop instantiation in OCCT code.
- Perhaps make the constructors private (or delete them with C++ 11), if they have been deprecated for some time.
TagsNo tags attached.
Test case number
Attached Filespatch file icon 0001-0029420-TopoDS_Builder-and-BRep_Builder-should-preve.patch (46,922 bytes) 2019-08-12 15:14

- Relationships

-  Notes
(0073365)
git (administrator)
2018-01-11 17:34

Branch CR29420 has been created by BenjaminBihler.

SHA-1: 04f851947dc7c2daf2a803880d22d17d14e571e6


Detailed log of new commits:

Author: Benjamin Bihler
Date: Thu Jan 11 15:26:57 2018 +0100

    0029420: TopoDS_Builder and BRep_Builder should prevent instantiation and their methods should be static
    
    Methods of TopoDS_Builder and BRep_Builder have been made static.
(0073366)
BenjaminBihler (developer)
2018-01-11 17:41

Andrey, do you agree? Should I implement the constructors and mark them as deprecated?
(0073377)
abv (manager)
2018-01-12 11:26

I agree that current implementation is not optimal, but does the proposed change bring any real benefits that would compensate expenses of many users who will need to modify their code (even if the change is trivial, it still has to be done)?
(0073380)
BenjaminBihler (developer)
2018-01-12 11:35

The performance/memory usage benefits are minimal: https://stackoverflow.com/questions/9490117/is-there-a-significant-inherent-cost-of-object-instantiation-in-c. [^] Avoiding confusion about builder state is a little bit more important.

If you say that deprecating the constructors is not worth the effort, I would at least propose to integrate my revision. Then everyone who want can use the methods without instantation and the compiler does not utter warnings for current code.
(0073389)
kgv (developer)
2018-01-12 14:47
edited on: 2018-01-12 14:48

I think it makes sense splitting the patch anyway - making methods static is harmless and useful as is. This would require just renaming bug description (title).

Additionally, patch can be extended (or followed by another bug) by switching existing code to avoid creating of Builder instance (which can be localized by temporarily deprecating constructor), but I'm afraid the patch would be tremendous for OCCT itself.

And finally, we may create a bug deprecating the constructor for everybody.
But this is questionable feature, because existing code will not benefit from this change but forcing application to migrate might require a lot of efforts (through this depends on application nature).

(0073390)
BenjaminBihler (developer)
2018-01-12 15:18

I have wondered whether regular expression rearch/replace could be used to quickly switch to calling static methods instead of members. Unfortunately this won't work, because I have immediately found many classes that store BRep_Builder instances and pass them around. See for example:

- BRepPrim_Builder(const BRep_Builder& B);
- BRepPrim_FaceBuilder(const BRep_Builder& B, const Handle(Geom_Surface)& S);
- ...
- BRepTools::Read (..., const BRep_Builder& B...)
and so on.

This makes refactoring really ugly, since method signatures would change. On the other hand my statement "the performance/memory usage benefits are minimal" seems wrong to me now. Storing instances as member fields and passing them around has indeed some costs, though they are low (I have not been aware of that). So the correct statement would be that the benefits are low, but they are more than minimal. ;-)
(0086180)
BenjaminBihler (developer)
2019-08-12 13:32

Has Kirill's note from 2018-01-12 been taken into account when setting the target version to "Unscheduled"?
(0086181)
BenjaminBihler (developer)
2019-08-12 13:48

And have you also taken into account that there is already a patch? Why not integrate it? The discussion was about whether further steps are necessary and/or useful... just skip them if you don't have the time, but use what is already there.
(0086185)
abv (manager)
2019-08-12 14:23

Benjamin, current version of the patch causes multiple warnings (854 in OCCT itself) during compilation of existing code, in most places where BRep_Builder is instantiated, since now such instances become unused local variables.

MSVC2015:

8>..\..\..\src\BRepTools\BRepTools_ReShape.cxx(386): warning C4101: 'B': unreferenced local variable
8>..\..\..\src\BRepTools\BRepTools_ReShape.cxx(440): warning C4101: 'aB': unreferenced local variable
8>..\..\..\src\BRepTools\BRepTools_ReShape.cxx(492): warning C4101: 'B': unreferenced local variable
8>..\..\..\src\BRepTools\BRepTools_Quilt.cxx(71): warning C4101: 'B': unreferenced local variable
8>..\..\..\src\BRepTools\BRepTools_Quilt.cxx(119): warning C4101: 'B': unreferenced local variable
8>..\..\..\src\BRepTools\BRepTools_Quilt.cxx(377): warning C4101: 'B': unreferenced local variable

Our policy is to have no warnings in OCCT code, thus the patch cannot be taken as is.

While I agree with you that making these methods static would be logical, I deem that practical benefit of this change (more convenience in writing new code) does not overweight inconvenience of updating all existing code (OCCT + user applciations). Note that we try not to break code compatibility without good reason, to allow the users to upgrade OCCT versions with less effort.

That said, I propose keeping the things as they are and closing the issue, unless you see it important (e.g. if you can measure expenses of instantiation of BRep_Builder class and show that they are non-negligible on some use case) and would like to complete the patch to avoid warnings.

Besides, I do not see much use in declaring constructors deprecated - that warning on unused local variable will indicate misuse in most cases.
(0086187)
BenjaminBihler (developer)
2019-08-12 14:28

I see. Thank you for the explanation.
(0086191)
BenjaminBihler (developer)
2019-08-12 14:35

In this case - should the issue be closed?
(0086192)
abv (manager)
2019-08-12 14:36

Yes, I will close it if you do not mind
(0086194)
BenjaminBihler (developer)
2019-08-12 14:43

Go! ;)
(0086198)
git (administrator)
2019-08-12 15:15

Branch CR29420 has been deleted by abv.

SHA-1: 04f851947dc7c2daf2a803880d22d17d14e571e6

- Issue History
Date Modified Username Field Change
2018-01-11 17:24 BenjaminBihler New Issue
2018-01-11 17:24 BenjaminBihler Assigned To => BenjaminBihler
2018-01-11 17:34 git Note Added: 0073365
2018-01-11 17:41 BenjaminBihler Note Added: 0073366
2018-01-11 17:41 BenjaminBihler Assigned To BenjaminBihler => abv
2018-01-11 17:42 BenjaminBihler Status new => feedback
2018-01-11 18:44 kgv Severity minor => integration request
2018-01-11 18:44 kgv Product Version 7.2.0 =>
2018-01-11 18:45 kgv Category OCCT:Foundation Classes => OCCT:Modeling Data
2018-01-11 18:45 kgv Summary TopoDS_Builder and BRepBuilder should prevent instantiation and their methods should be static => Modeling Data - TopoDS_Builder and BRepBuilder should prevent instantiation and their methods should be static
2018-01-12 11:26 abv Note Added: 0073377
2018-01-12 11:35 BenjaminBihler Note Added: 0073380
2018-01-12 14:47 kgv Note Added: 0073389
2018-01-12 14:48 kgv Note Edited: 0073389 View Revisions
2018-01-12 14:48 kgv Note Edited: 0073389 View Revisions
2018-01-12 15:18 BenjaminBihler Note Added: 0073390
2019-08-12 13:11 msv Target Version 7.4.0 => Unscheduled
2019-08-12 13:32 BenjaminBihler Note Added: 0086180
2019-08-12 13:48 BenjaminBihler Note Added: 0086181
2019-08-12 14:23 abv Note Added: 0086185
2019-08-12 14:23 abv Assigned To abv => BenjaminBihler
2019-08-12 14:28 BenjaminBihler Note Added: 0086187
2019-08-12 14:35 BenjaminBihler Note Added: 0086191
2019-08-12 14:35 BenjaminBihler Assigned To BenjaminBihler => abv
2019-08-12 14:36 abv Note Added: 0086192
2019-08-12 14:43 BenjaminBihler Note Added: 0086194
2019-08-12 15:03 abv Status feedback => closed
2019-08-12 15:03 abv Resolution open => fixed
2019-08-12 15:03 abv Target Version Unscheduled =>
2019-08-12 15:14 abv File Added: 0001-0029420-TopoDS_Builder-and-BRep_Builder-should-preve.patch
2019-08-12 15:15 git Note Added: 0086198


Copyright © 2000 - 2019 MantisBT Team
Powered by Mantis Bugtracker