MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0024533Community[OCCT] OCCT:Foundation Classespublic2014-01-15 21:352014-11-11 12:59
ReporterRoman Lygin 
Assigned Tobugmaster 
PrioritynormalSeverityintegration request 
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version[OCCT] 6.7.0 
Target Version[OCCT] 6.8.0Fixed in Version[OCCT] 6.8.0 
Summary0024533: Use 0 to check null handle instead of UndefinedHandleAccess
Descriptionsistent used specific non-null value
(e.g. 0xfefd0000 on 32 bit platforms) to check nullified handles.

This makes a lot of pain and risk for the following situations:

1. 'bool casting' -like constructs
Handle_MyType h = ReturnObject();
if (h) {
 h->DoSomething ();
}

2. comparing with explicit NULL
if (h != 0) {
  h->DoSomething()
}

0000001 and 0000002 are frequent newbies errors which are frequently reported on the forum, until someone advices to replace with !h.IsNull().

3. refactoring codes changing API from pointers to handles which can be human error-prone:
old:
MyType* p = ReturnObject();
... //many many lines below
if (p) {
  p->DoSomething();
}

updated to (e.g. due changed API of ReturnedObject()):
Handle_MyType p = ReturnObject();
... //many many lines below
if (p) { //remained unnoticed due to human error
}

#3 can be met in complex project during refactoring.


Note that boost::shared_ptr and std::shared_ptr do use 0 to identify uninitialized or nullified value.

The only related comment I remember is one of Andrey dated to November 2008,
http://opencascade.blogspot.ru/2008/11/open-cascade-handles-lets-handleem-part_21.html?showComment=1227264000000#c1579880060849701201: [^]

1. There is some utility of having UndefinedHandleAddress not equal to zero. I have seen a few cases when one static variable is constructed by reference to another static variable which might be not-yet-initialized at the moment. Now in such case you get immediately Access Violation signal at null address; if UndefinedHandleAddress were null this problem might remain unnoticed.

One idea (thanks to AGV) is to set this value to 0x01 -- this address should be bad for all platforms. Though not completely sure...

Even if there is a risk for such case, this is still a corner case, much less probable than any of 0000001, 0000002, #3 above.

So use of UndefinedHandleAddress is discontinued and replaced with 0. Moreover, comparing to zero is likely a cheaper operation
performance-wise, with respect to comparing to a constant (the latter likely invokes extra subtraction).
Given that handle access are often on critical paths, this will also be valuable.


In addition, two other changes were added:
- some minor code clean up (removal of redundant Standard_EXPORT)
- replacing return type of comparison operator from int with bool. This is to avoid warnings in instantiations of std::unordered_set, unordered_map
using handles as keys where std::equal_to class would be instantiated with Handle_Standard_Transient.
Steps To ReproduceSee enclosed test case
TagsNo tags attached.
Test case numberbugs fclasses(002) bug24533
Attached Filescxx file icon test.cxx (533 bytes) 2014-01-15 21:35

- Relationships

-  Notes
(0027507)
Roman Lygin (developer)
2014-01-15 23:19

Fix pushed to the git repository
(0027508)
Roman Lygin (developer)
2014-01-15 23:32

Fix in WOK git repository has also been pushed (the package MS). The file CPPExt_Standard.edl remained unchanged as it seems to unused.
(0027526)
abv (manager)
2014-01-17 06:53

Reviewed, please test. Note that I have pushed one more comment providing description of changes made and eliminating remaining definitions of _OCC64 macro.
(0027527)
Roman Lygin (developer)
2014-01-17 09:03

Andrey, thanks for a further update.
I have intentionally left _OCC64 macro unchanged (outside of the modified files) to reserve it for possible OCC needs. If some 32/64-specific codes were required it would be possible to reuse it. Of course, not a big deal - one can always use OCC-invariant constructs (like those used in Standard_Macro.hxx to define _OCC64 itself).

P.S. Perhaps time to process 0022928 at last ;-) ?
(0027528)
Roman Lygin (developer)
2014-01-17 09:06

By the way, you might want then to update WOK repository as well - MS Project generator seems to have _OCC64 hardcoded in WOKTclLib/templates.
Also please double-check that SALOME and other projects do not rely on it.
(0027529)
abv (manager)
2014-01-17 10:18

Thank you Roman for the hint -- I did not remember that project templates have been moved to subfolder in WOK, and have not found them... I have pushed correction to WOK repo.
(0027626)
mkv (tester)
2014-01-23 12:47

Dear BugMaster,

Branch CR24533 (and products from GIT master) was compiled on Linux and Windows platforms and tested.
SHA-1: 83ab8f2aa0de13d94c332635c3d710d2830dc15c

Number of compiler warnings:

occt component :
Linux: 48 (48 on master)
Windows: 1 (1 on master)

products component :
Linux: 12 (12 on master)
Windows: 2 (2 on master)

Regressions/Differences:
No regressions/differences

Testing cases:
bugs fclasses(002) bug24533 - OK.

Testing on Linux:
Total MEMORY difference: 370680548 / 366211348
Total CPU difference: 44012.140000000276 / 44012.79000000015

Testing on Windows:
Total MEMORY difference: 416934816 / 417396220
Total CPU difference: 30946.265625 / 32472.65625

There are not differences in images found by testdiff.
(0028225)
bugmaster (administrator)
2014-03-12 14:41

Fix for WOK has been integrated into master of occt-wok repository
(0033134)
Roman Lygin (developer)
2014-10-15 15:08

Confirmed as closed. Cannot close myself.

- Related Changesets
occt: master 4d9421a9
Timestamp: 2014-01-23 09:37:50
Author: Roman Lygin
Committer: bugmaster
Details ] Diff ]
0024533: Use 0 to check null handle instead of UndefinedHandleAccess

Handle classes now use 0 as invalid value for pointer instead of custom (and platform-dependent) value like 0xfefd0000.
Compiler macros UndefinedHandleAddress and _OCC64 are eliminated.
mod - CMakeLists.txt Diff ] File ]
mod - src/LDOM/LDOM_XmlReader.cxx Diff ] File ]
mod - src/OSD/OSD_signal.cxx Diff ] File ]
mod - src/Standard/Handle_Standard_Persistent.hxx Diff ] File ]
mod - src/Standard/Handle_Standard_Transient.cxx Diff ] File ]
mod - src/Standard/Handle_Standard_Transient.hxx Diff ] File ]
mod - src/Standard/Standard_Macro.hxx Diff ] File ]
mod - src/Standard/Standard_MMgrOpt.cxx Diff ] File ]

- Issue History
Date Modified Username Field Change
2014-01-15 21:35 Roman Lygin New Issue
2014-01-15 21:35 Roman Lygin Assigned To => abv
2014-01-15 21:35 Roman Lygin File Added: test.cxx
2014-01-15 23:19 Roman Lygin Note Added: 0027507
2014-01-15 23:19 Roman Lygin Status new => resolved
2014-01-15 23:32 Roman Lygin Note Added: 0027508
2014-01-17 06:53 abv Note Added: 0027526
2014-01-17 06:53 abv Assigned To abv => bugmaster
2014-01-17 06:53 abv Status resolved => reviewed
2014-01-17 07:21 mkv Assigned To bugmaster => mkv
2014-01-17 09:03 Roman Lygin Note Added: 0027527
2014-01-17 09:06 Roman Lygin Note Added: 0027528
2014-01-17 10:18 abv Note Added: 0027529
2014-01-23 12:47 mkv Note Added: 0027626
2014-01-23 12:48 mkv Test case number => bugs fclasses(002) bug24533
2014-01-23 12:48 mkv Assigned To mkv => bugmaster
2014-01-23 12:48 mkv Status reviewed => tested
2014-01-24 15:35 bugmaster Changeset attached => occt master 4d9421a9
2014-01-24 15:35 bugmaster Status tested => verified
2014-01-24 15:35 bugmaster Resolution open => fixed
2014-01-27 11:41 bugmaster Target Version => 6.7.1
2014-03-12 14:41 bugmaster Note Added: 0028225
2014-04-04 12:06 abv Target Version 6.7.1 => 6.8.0
2014-10-15 15:08 Roman Lygin Assigned To bugmaster => Roman Lygin
2014-10-15 15:08 Roman Lygin Assigned To Roman Lygin => bugmaster
2014-10-15 15:08 Roman Lygin Note Added: 0033134
2014-11-11 12:44 user533 Fixed in Version => 6.8.0
2014-11-11 12:59 user533 Status verified => closed


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker