View Issue Details

IDProjectCategoryView StatusLast Update
0003526Slicer4Core: Usabilitypublic2015-09-01 10:26
Reporternicole Assigned Tonicole  
PrioritynormalSeverityminorReproducibilityhave not tried
Status acknowledgedResolutionopen 
Product Version 
Target VersionFixed in Version 
Summary0003526: User proper null pointer checking instead of assert
Description

From the slicer-devel mailing list, there's consensus on changing uses of assert to null pointer checking and vtkErrorMacro and returning rather than crashing in debug mode.

The nightly testing assumptions may need to be adjusted to get correct test failures and to ensure that developers fix problems that the asserts were catching.

Additional Information

Email thread:
http://slicer-devel.65872.n3.nabble.com/Use-proper-null-pointer-checking-instead-of-assert-td4030168.html

TagsNo tags attached.

Activities

nicole

nicole

2013-12-19 13:03

administrator   ~0010451

Last edited: 2013-12-19 13:12

Did it in two commits, one removing the C++ asserts:
https://github.com/naucoin/Slicer/commit/e52351dcba2588e885ef8054aa1c89b91c024aea
One removing Q_ASSERT:
https://github.com/naucoin/Slicer/commit/f1ad1d91fb0c2cfc2e3e4e56db7bbcc39c1be011

Ran make Experimental uploads as was making changes, and caught a few logic errors. Last one after rebasing:
http://slicer.cdash.org/viewTest.php?onlyfailed&buildid=179119
The volume rendering scene close test is passing now, though it prints out an ERROR via the qCritical() call. Others are also failing on the trunk.

Posting to the dev list for more feedback, especially on the qMRMLSceneModel

pinter

pinter

2013-12-19 15:12

developer   ~0010452

What I found useful and what makes the log messages consistent is to prefix all messages from std and Qt with class and function name, like
qCritical() << "qSlicerClassName::methodName: Error message";
I use this throughout my code, because the vtkErrorMacro and siblings all prefix the messages with the class name, so this way the error messages look very similar (which makes them easier to read), and immediately knowing the place of error also helps.

I also have been thinking about the std::cerr usage where there is no VTK object whatsoever that can be used for vtkErrorWithObjectMacro and such, and found a macro named vtkGenericWarningMacro. I haven't had time to look into that, just made a note about it, so this might not work, but is worth noting, in case it does.

jcfr

jcfr

2013-12-19 17:38

administrator   ~0010455

Should all the qCritical print outs use the ERROR string?
Yes - but the developer shouldn't add the text

Should there be more data on the file/method names in the qCritical strings?
Yes - but that should be automatically done

A lot of the asserts in the vtk files were in places where I couldn't
use vtkErrorMacro, so I just used piping to std::cerr where the
vtkErrorWithObjectMacro wasn't working either. Is there a better way?
No - the approach reported by Csaba should be investigated.

Waiting Slicer transition to Qt5, to associate more information with qCritical/qWarning/qDebug, the QMessageLogger class now available in Qt5 could be backported into CTK. See http://qt-project.org/doc/qt-5.0/qtcore/qmessagelogger.html#details

nicole

nicole

2014-03-06 11:11

administrator   ~0011262

Rebase topic branch and integrate on Mar 6 or 7, 2014.

Issue History

Date Modified Username Field Change
2013-12-09 12:33 nicole New Issue
2013-12-09 12:33 nicole Status new => assigned
2013-12-09 12:33 nicole Assigned To => nicole
2013-12-19 13:03 nicole Note Added: 0010451
2013-12-19 13:12 nicole Note Edited: 0010451
2013-12-19 15:12 pinter Note Added: 0010452
2013-12-19 17:38 jcfr Note Added: 0010455
2014-03-06 11:11 nicole Note Added: 0011262
2014-03-07 09:50 pieper Target Version Slicer 4.4.0 => Slicer 4.5.0-1
2014-05-13 12:52 jcfr Status assigned => acknowledged
2015-09-01 10:26 nicole Target Version Slicer 4.5.0-1 =>