Skip to content

COMP: Exceptions should be caught by constant reference#1492

Merged
dzenanz merged 5 commits into
InsightSoftwareConsortium:masterfrom
hjmjohnson:builld-warning-exception
Dec 14, 2019
Merged

COMP: Exceptions should be caught by constant reference#1492
dzenanz merged 5 commits into
InsightSoftwareConsortium:masterfrom
hjmjohnson:builld-warning-exception

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

Modules/Core/SpatialObjects/test/itkContourSpatialObjectTest.cxx:244:31:
warning: catching polymorphic type ‘class itk::ExceptionObject’ by value [-Wcatch-value=]

Exceptions should be caught by constant reference, not by value.

@hjmjohnson hjmjohnson self-assigned this Dec 14, 2019
Modules/Core/SpatialObjects/test/itkContourSpatialObjectTest.cxx:244:31:
warning: catching polymorphic type ‘class itk::ExceptionObject’ by value [-Wcatch-value=]

Catching by value is problematic in the face of inheritance hierarchies.

https://clang.llvm.org/extra/clang-tidy/checks/misc-throw-by-value-catch-by-reference.html
https://wiki.sei.cmu.edu/confluence/display/cplusplus/ERR61-CPP.+Catch+exceptions+by+lvalue+reference
@hjmjohnson hjmjohnson force-pushed the builld-warning-exception branch from be9f2d4 to fcdac9b Compare December 14, 2019 12:35
The second argument to the stream insertion operator
should be a constant reference or pointer.

operator<<(std::ostream & os, const EventObject & e)

Stream insertion should not modify the object
being inserted.
When throwing exceptions, getting the data object pointer
should be always be constant.
@hjmjohnson hjmjohnson requested review from dzenanz, mseng10 and thewtex and removed request for mseng10 December 14, 2019 17:42
Copy link
Copy Markdown
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Do we also need to update the software guide with recommendations and examples regarding catching exceptions by reference?

@hjmjohnson
Copy link
Copy Markdown
Member Author

Great! Do we also need to update the software guide with recommendations and examples regarding catching exceptions by reference?

That is "C++" best practices, so I think it would be OK, but not necessary. I'd be cautious to not be too TLDR; regarding details like this.

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Dec 14, 2019

What I meant, update the examples in the software guide to follow this, and if there are statements opposite to this update or remove them. I did not think of expansion.

@hjmjohnson
Copy link
Copy Markdown
Member Author

@dzenanz Oh. the Examples are already updated in the PR. I am working on a bash script to detect this in the SphinxExamples too, but that might be a few weeks before I get to it.

@dzenanz
Copy link
Copy Markdown
Member

dzenanz commented Dec 14, 2019

I meant update code snippets such as these.

@hjmjohnson
Copy link
Copy Markdown
Member Author

@hjmjohnson
Copy link
Copy Markdown
Member Author

Python failures are not related to these changes. @thewtex something is happening related to swig.

https://open.cdash.org/viewTest.php?onlyfailed&buildid=6252493

Would @thewtex or @dzenanz merge when satisfied that python failures are some other problem?

@dzenanz dzenanz merged commit 42bfdd5 into InsightSoftwareConsortium:master Dec 14, 2019
@thewtex
Copy link
Copy Markdown
Member

thewtex commented Dec 15, 2019

https://open.cdash.org/viewTest.php?onlyfailed&buildid=6252493

Something that is not reproducible sometimes causes these test failures on Linux CI builds.

@hjmjohnson hjmjohnson deleted the builld-warning-exception branch February 13, 2020 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants