View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0027870Open CASCADEOCCT:Modeling Algorithmspublic2016-09-14 00:462021-04-10 10:21
ReporterabvAssigned Toapn  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
Target Version7.1.0Fixed in Version7.1.0 
Summary0027870: Modeling - refactoring of HLR algorithms
DescriptionCurrent implementation of HLR algorithms is ugly and undebuggable, due to extensive usage of low-level types (Standard_Address) and macros involving low-level casts to access particular data elements, instead of using typed data structures and accessing them using names. As result, no static type checking is performed, and debugger cannot be used normally to view content of "variables" (actually macros) used throughout the code.

See e.g. HLRAlgo_PolyData.cxx. For instance, method HLRAlgo_PolyData::UpdateGlobalMinMax() should take Bnd_Box or equivalent structure, or at least double[2][3] as its argument, and not Standard_Address!
Steps To ReproduceN/A
TagsNo tags attached.
Test case numberNot needed

Relationships

parent of 0032292 closedbugmaster Open CASCADE Coding Rules - improve HLRBRep_PolyAlgo readability part 1 

Activities

msv

2016-09-26 12:31

developer   ~0058154

Dear Julia, please take care of this issue.

git

2016-10-07 18:31

administrator   ~0058496

Branch CR27870 has been created by abk.

SHA-1: 4d969765e4ac4786dcb3e51954d12918067600a6


Detailed log of new commits:

Author: abk
Date: Wed Oct 5 13:49:44 2016 +0300

    HLRAlgo: Standard_Real[]
    
    HLRAlgo_EdgesBlock::MinMaxIndices
    
    PINod
    
    PISeg
    
    4
    
    3
    
    2
    
    1

git

2016-10-07 18:42

administrator   ~0058497

Branch CR27870 has been updated forcibly by abk.

SHA-1: cfe0e49fd55834f26a57b82e59adfbaa47169b35

git

2016-10-07 19:14

administrator   ~0058502

Branch CR27870 has been updated forcibly by abk.

SHA-1: 900b67e5ceac77518e2c283782a53653e4808b6c

abv

2016-10-10 07:41

manager   ~0058511

Last edited: 2016-10-10 09:09

 Some remarks on first changes:

1. In HLRAlgo_BiPoint::HLRAlgo_BiPoint(), when initializing myPoints:

> myPoints.Pnt1 = gp_XYZ(X1, Y1, Z1);

can be better replaced by

> myPoints.Pnt1.SetCoord (X1, Y1, Z1);

2. In declaration of HLRAlgo::UpdateMinMax(), it is better to specify dimension of arrays: Standard_Real Min[16]. Casts of Min to Standard_Real* are not needed.

3. In HLRAlgo_BiPoint.hxx: (a) when defining a structure, it is better to define each field on its own line, (b) when defining single-line inline method, keep "{" and "}" on separate lines (unless whole method fits single line).

4. HLRAlgo_PolyData.hxx: why enum HLR_PolyMask is defined here? It is not used in this header at all! Please define it where it is used, as locally as possible.

git

2016-10-17 21:18

administrator   ~0058829

Branch CR27870 has been updated forcibly by abk.

SHA-1: aba21216a01a9b6a7f00cdfd64f08cfa8162d29c

msv

2016-10-25 12:04

developer   ~0059064

Dear Julia, please review the fix.

jgv

2016-10-27 12:48

developer   ~0059245

My remarks:

1. HLRAlgo_PolyInternalData.cxx
line 323: why
Nod3RValues.Normal *= 1 / aNorm;
but not
Nod3RValues.Normal = aXYZ / aNorm; ?

2. HLRAlgo_PolyInternalData.cxx lines 698, 739;
HLRBRep_PolyAlgo.cxx lines 2471, 2942, 3125, 3135 :
there is a risk of usage of null pointer, it is better to avoid it like it is done in fix to OCC25214.

msv

2016-10-27 15:22

developer   ~0059260

I propose to rebase the branch on CR25214, as it has already been tested.

git

2016-10-28 16:46

administrator   ~0059357

Branch CR27870 has been updated forcibly by abk.

SHA-1: 6490d024bddca285f14032f9dc58d2d6148b63e8

abk

2016-10-28 16:52

developer   ~0059362

The remarks were considered, the branch was rebased to the master.
Please review ASAP.

abk

2016-10-28 16:57

developer   ~0059363

Please test.

git

2016-10-28 18:41

administrator   ~0059382

Branch CR27870 has been updated forcibly by apv.

SHA-1: 847fe70ad1ad584ea140d6b8fffd9944c64eb1a2

apv

2016-10-28 18:42

tester   ~0059383

Branch CR27870 has been rebased on the current master

apv

2016-11-01 15:03

tester   ~0059695

Dear BugMaster,

Branch CR27870 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested.
SHA-1: 847fe70ad1ad584ea140d6b8fffd9944c64eb1a2

Number of compiler warnings:
occt component:
   Linux: 0 (0 on master)
   Windows: 0 (0 on master)
   MasOS: 0 (0 on master)
products component:
   Linux: 63
   Windows: 0
   MacOS: 1149

Regressions/Differences:
Not detected

Testing cases:
Not needed

Testing on Linux:
Total MEMORY difference: 91024361 / 90749168 [+0.30%]
Total CPU difference: 19467.219999999903 / 19313.639999999898 [+0.80%]

Testing on Windows:
Total MEMORY difference: 57306156 / 57321116 [-0.03%]
Total CPU difference: 17749.575378598707 / 18220.18359529866 [-2.58%]

apn

2016-11-02 18:32

administrator   ~0059812

There are compilation errors in debug mode (build with preprocessor definition OCCT_DEBUG):

Building CXX object src/TKHLR/CMakeFiles/TKHLR.dir/__/HLRBRep/HLRBRep_ShapeBounds.cxx.o
/Users/mnt/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:906:12: error: use of undeclared identifier 'Nod11NrmX'
              if (Nod11NrmX*Nod12NrmX +
                  ^
/Users/mnt/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:906:22: error: use of undeclared identifier 'Nod12NrmX'
              if (Nod11NrmX*Nod12NrmX +
                            ^
/Users/mnt/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:907:5: error: use of undeclared identifier 'Nod11NrmY'
                  Nod11NrmY*Nod12NrmY +
                  ^
/Users/mnt/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:907:15: error: use of undeclared identifier 'Nod12NrmY'
                  Nod11NrmY*Nod12NrmY +
                            ^
/Users/mnt/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:908:15: error: use of undeclared identifier 'Nod12NrmZ'
                  Nod11NrmZ*Nod12NrmZ < 0) {
                            ^
/Users/mnt/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:908:5: error: use of undeclared identifier 'Nod11NrmZ'
                  Nod11NrmZ*Nod12NrmZ < 0) {
                  ^
/Users/mnt/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:1093:12: error: use of undeclared identifier 'Nod21NrmX'
              if (Nod21NrmX*Nod22NrmX +
                  ^
/Users/mnt/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:1093:22: error: use of undeclared identifier 'Nod22NrmX'
              if (Nod21NrmX*Nod22NrmX +
                  ^
/Users/mnt/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:1094:5: error: use of undeclared identifier 'Nod21NrmY'
                  Nod21NrmY*Nod22NrmY +
                  ^
/Users/mnt/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:1094:15: error: use of undeclared identifier 'Nod22NrmY'
                  Nod21NrmY*Nod22NrmY +
                  ^
/Users/mnt/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:1095:15: error: use of undeclared identifier 'Nod22NrmZ'
                  Nod21NrmZ*Nod22NrmZ < 0) {
                  ^
/Users/mnt/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:1095:5: error: use of undeclared identifier 'Nod21NrmZ'
                  Nod21NrmZ*Nod22NrmZ < 0) {
                  ^
/Users/mnt/builds/IR-2016-11-02_IR-2016-11-02/MacOS-deb/OCCT/src/HLRBRep/HLRBRep_PolyAlgo.cxx:2313:11: error: use of undeclared identifier 'Tri1Node1'
                cout << Tri1Node1 << " modifies : DX,DY ";

git

2016-11-03 13:09

administrator   ~0059843

Branch CR27870 has been updated forcibly by abk.

SHA-1: e02a4642317745189856c90e5c3acda1abde4560

abk

2016-11-03 13:16

developer   ~0059847

The cause of the compilation errors in the debug mode was fixed.
Please test again.

git

2016-12-07 11:30

administrator   ~0061249

Branch CR27870 has been deleted by kgv.

SHA-1: e02a4642317745189856c90e5c3acda1abde4560

Related Changesets

occt: master 681f3919

2016-10-05 10:49:44

abk


Committer: apn Details Diff		 0027870: Modeling - refactoring of HLR algorithms

Toolkit 'TKHLR' was fully refactored for 'Standard_Address' and macros except about half of package 'HLRBREP' there 'Standard_Address' is used through the 'generic' mechanism.		 Affected Issues
0027870
mod - src/Contap/Contap_ContAna.cxx Diff File
mod - src/Contap/Contap_Contour.cxx Diff File
mod - src/Contap/Contap_SurfFunction.cxx Diff File
mod - src/DBRep/DBRep_HideData.cxx Diff File
mod - src/HLRAlgo/FILES Diff File
mod - src/HLRAlgo/HLRAlgo.cxx Diff File
mod - src/HLRAlgo/HLRAlgo.hxx Diff File
mod - src/HLRAlgo/HLRAlgo_BiPoint.cxx Diff File
mod - src/HLRAlgo/HLRAlgo_BiPoint.hxx Diff File
rm - src/HLRAlgo/HLRAlgo_Coincidence.cxx Diff File
mod - src/HLRAlgo/HLRAlgo_Coincidence.hxx Diff File
rm - src/HLRAlgo/HLRAlgo_Coincidence.lxx Diff File
mod - src/HLRAlgo/HLRAlgo_EdgeIterator.cxx Diff File
mod - src/HLRAlgo/HLRAlgo_EdgeIterator.hxx Diff File
mod - src/HLRAlgo/HLRAlgo_EdgeIterator.lxx Diff File
mod - src/HLRAlgo/HLRAlgo_EdgesBlock.cxx Diff File
mod - src/HLRAlgo/HLRAlgo_EdgesBlock.hxx Diff File
mod - src/HLRAlgo/HLRAlgo_PolyAlgo.cxx Diff File
mod - src/HLRAlgo/HLRAlgo_PolyAlgo.hxx Diff File
mod - src/HLRAlgo/HLRAlgo_PolyData.cxx Diff File
mod - src/HLRAlgo/HLRAlgo_PolyData.hxx Diff File
mod - src/HLRAlgo/HLRAlgo_PolyData.lxx Diff File
mod - src/HLRAlgo/HLRAlgo_PolyHidingData.hxx Diff File
rm - src/HLRAlgo/HLRAlgo_PolyHidingData.lxx Diff File
mod - src/HLRAlgo/HLRAlgo_PolyInternalData.cxx Diff File
mod - src/HLRAlgo/HLRAlgo_PolyInternalData.hxx Diff File
mod - src/HLRAlgo/HLRAlgo_PolyInternalNode.hxx Diff File
rm - src/HLRAlgo/HLRAlgo_PolyInternalNode.lxx Diff File
mod - src/HLRAlgo/HLRAlgo_PolyInternalSegment.hxx Diff File
rm - src/HLRAlgo/HLRAlgo_PolyInternalSegment.lxx Diff File
add - src/HLRAlgo/HLRAlgo_PolyMask.hxx Diff File
mod - src/HLRAlgo/HLRAlgo_PolyShellData.cxx Diff File
mod - src/HLRAlgo/HLRAlgo_PolyShellData.hxx Diff File
mod - src/HLRAlgo/HLRAlgo_PolyShellData.lxx Diff File
mod - src/HLRAlgo/HLRAlgo_TriangleData.hxx Diff File
rm - src/HLRAlgo/HLRAlgo_TriangleData.lxx Diff File
mod - src/HLRAlgo/HLRAlgo_WiresBlock.cxx Diff File
mod - src/HLRAlgo/HLRAlgo_WiresBlock.hxx Diff File
rm - src/HLRAlgo/HLRAlgo_WiresBlock.lxx Diff File
mod - src/HLRBRep/HLRBRep_BiPnt2D.hxx Diff File
mod - src/HLRBRep/HLRBRep_CLProps.hxx Diff File
mod - src/HLRBRep/HLRBRep_CLPropsATool.hxx Diff File
mod - src/HLRBRep/HLRBRep_CLPropsATool.lxx Diff File
mod - src/HLRBRep/HLRBRep_CLProps_0.cxx Diff File
mod - src/HLRBRep/HLRBRep_Curve.cxx Diff File
mod - src/HLRBRep/HLRBRep_Curve.hxx Diff File
mod - src/HLRBRep/HLRBRep_Curve.lxx Diff File
mod - src/HLRBRep/HLRBRep_Data.cxx Diff File
mod - src/HLRBRep/HLRBRep_Data.hxx Diff File
mod - src/HLRBRep/HLRBRep_EdgeData.cxx Diff File
mod - src/HLRBRep/HLRBRep_EdgeData.hxx Diff File
mod - src/HLRBRep/HLRBRep_EdgeData.lxx Diff File
mod - src/HLRBRep/HLRBRep_InternalAlgo.cxx Diff File
mod - src/HLRBRep/HLRBRep_Intersector.cxx Diff File
mod - src/HLRBRep/HLRBRep_Intersector.hxx Diff File
mod - src/HLRBRep/HLRBRep_PolyAlgo.cxx Diff File
mod - src/HLRBRep/HLRBRep_PolyAlgo.hxx Diff File
mod - src/HLRBRep/HLRBRep_PolyHLRToShape.cxx Diff File
mod - src/HLRBRep/HLRBRep_ShapeBounds.cxx Diff File
mod - src/HLRBRep/HLRBRep_ShapeBounds.hxx Diff File
mod - src/HLRBRep/HLRBRep_ShapeBounds.lxx Diff File
mod - src/HLRBRep/HLRBRep_Surface.cxx Diff File
mod - src/HLRBRep/HLRBRep_Surface.hxx Diff File
mod - src/HLRBRep/HLRBRep_Surface.lxx Diff File
mod - src/HLRTest/HLRTest_DrawablePolyEdgeTool.cxx Diff File
mod - src/HLRTopoBRep/HLRTopoBRep_Data.cxx Diff File
mod - src/HLRTopoBRep/HLRTopoBRep_Data.hxx Diff File
mod - src/HLRTopoBRep/HLRTopoBRep_DSFiller.cxx Diff File
mod - src/StdPrs/StdPrs_HLRPolyShape.cxx Diff File

Issue History

Date Modified Username Field Change
2016-09-14 00:46 abv New Issue
2016-09-14 00:46 abv Assigned To => msv
2016-09-26 12:31 msv Note Added: 0058154
2016-09-26 12:31 msv Assigned To msv => jgv
2016-09-26 12:31 msv Status new => assigned
2016-10-04 15:57 abv Assigned To jgv => abk
2016-10-07 18:31 git Note Added: 0058496
2016-10-07 18:42 git Note Added: 0058497
2016-10-07 19:14 git Note Added: 0058502
2016-10-10 07:41 abv Note Added: 0058511
2016-10-10 09:09 abv Note Edited: 0058511
2016-10-17 21:18 git Note Added: 0058829
2016-10-17 21:21 abk Assigned To abk => abv
2016-10-17 21:21 abk Status assigned => resolved
2016-10-17 21:21 abk Steps to Reproduce Updated
2016-10-17 23:13 abv Assigned To abv => jgv
2016-10-25 12:04 msv Note Added: 0059064
2016-10-27 12:48 jgv Note Added: 0059245
2016-10-27 12:48 jgv Status resolved => assigned
2016-10-27 12:48 jgv Assigned To jgv => abk
2016-10-27 15:22 msv Note Added: 0059260
2016-10-28 16:46 git Note Added: 0059357
2016-10-28 16:52 abk Note Added: 0059362
2016-10-28 16:52 abk Assigned To abk => jgv
2016-10-28 16:52 abk Status assigned => resolved
2016-10-28 16:52 abk Steps to Reproduce Updated
2016-10-28 16:57 abk Note Added: 0059363
2016-10-28 16:57 abk Assigned To jgv => bugmaster
2016-10-28 16:57 abk Status resolved => reviewed
2016-10-28 17:09 apv Assigned To bugmaster => apv
2016-10-28 18:41 git Note Added: 0059382
2016-10-28 18:42 apv Note Added: 0059383
2016-11-01 13:27 apv Test case number => Not needed
2016-11-01 15:03 apv Note Added: 0059695
2016-11-01 15:03 apv Assigned To apv => bugmaster
2016-11-01 15:03 apv Status reviewed => tested
2016-11-02 18:32 apn Note Added: 0059812
2016-11-02 18:32 apn Assigned To bugmaster => abk
2016-11-02 18:32 apn Status tested => feedback
2016-11-03 13:09 git Note Added: 0059843
2016-11-03 13:16 abk Note Added: 0059847
2016-11-03 13:16 abk Assigned To abk => apn
2016-11-03 13:16 abk Status feedback => reviewed
2016-11-03 17:06 apn Status reviewed => tested
2016-11-03 17:10 apn Changeset attached => occt master 681f3919
2016-11-03 17:10 apn Status tested => verified
2016-11-03 17:10 apn Resolution open => fixed
2016-12-07 11:30 git Note Added: 0061249
2016-12-09 16:28 aiv Status verified => closed
2016-12-09 16:40 aiv Fixed in Version => 7.1.0
2019-02-14 12:04 bugmaster Project Internal => Open CASCADE
2021-04-10 10:21 kgv Relationship added parent of 0032292