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:242018-01-12 15:18
Assigned Toabv 
PrioritynormalSeverityintegration request 
PlatformOSOS Version
Product Version 
Target Version[OCCT] 7.3.0*Fixed 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 Files

- Relationships

-  Notes
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.
BenjaminBihler (developer)
2018-01-11 17:41

Andrey, do you agree? Should I implement the constructors and mark them as deprecated?
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)?
BenjaminBihler (developer)
2018-01-12 11:35

The performance/memory usage benefits are minimal: [^] 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.
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).

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. ;-)

- 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

Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker