View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0029420 | Community | OCCT:Modeling Data | public | 2018-01-11 17:24 | 2019-08-12 15:15 |
| Reporter | BenjaminBihler | Assigned To | |||
| Priority | normal | Severity | integration request | ||
| Status | closed | Resolution | fixed | ||
| Summary | 0029420: Modeling Data - TopoDS_Builder and BRepBuilder should prevent instantiation and their methods should be static | ||||
| Description | TopoDS_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. | ||||
| Tags | No tags attached. | ||||
| Test case number | |||||
|
|
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. |
|
|
Andrey, do you agree? Should I implement the constructors and mark them as deprecated? |
|
|
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)? |
|
|
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. |
|
|
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). |
|
|
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. ;-) |
|
|
Has Kirill's note from 2018-01-12 been taken into account when setting the target version to "Unscheduled"? |
|
|
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. |
|
|
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. |
|
|
I see. Thank you for the explanation. |
|
|
In this case - should the issue be closed? |
|
|
Yes, I will close it if you do not mind |
|
|
Go! ;) |
|
2019-08-12 15:14 manager |
0001-0029420-TopoDS_Builder-and-BRep_Builder-should-preve.patch (46,922 bytes) |
|
|
Branch CR29420 has been deleted by abv. SHA-1: 04f851947dc7c2daf2a803880d22d17d14e571e6 |
| 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 |
|
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 | |
| 2018-01-12 14:48 | kgv | Note Edited: 0073389 | |
| 2018-01-12 15:18 | BenjaminBihler | Note Added: 0073390 | |
| 2019-08-12 13:11 |
|
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 |
|
Note Added: 0086185 | |
| 2019-08-12 14:23 |
|
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 |
|
Note Added: 0086192 | |
| 2019-08-12 14:43 | BenjaminBihler | Note Added: 0086194 | |
| 2019-08-12 15:03 |
|
Status | feedback => closed |
| 2019-08-12 15:03 |
|
Resolution | open => fixed |
| 2019-08-12 15:03 |
|
Target Version | Unscheduled => |
| 2019-08-12 15:14 |
|
File Added: 0001-0029420-TopoDS_Builder-and-BRep_Builder-should-preve.patch | |
| 2019-08-12 15:15 | git | Note Added: 0086198 |
