Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STYLE: Catch exceptions by _const_ reference #3419

Conversation

N-Dekker
Copy link
Contributor

Replaced catch \((\w+) & with catch (const $1 &, using Visual Studio 2019, Find and Replace, Replace in Files, Use regular expressions.

Following C++ Core Guidelines, April 10, 2022, which says that it is "typically better" to catch by const reference than to catch by non-const reference. At https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#e15-throw-by-value-catch-exceptions-from-a-hierarchy-by-reference

Discussed at C++ Core Guidelines issue isocpp/CppCoreGuidelines#518 "Catch exceptions by const reference?", from February 3, 2016.

@github-actions github-actions bot added area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module type:Style Style changes: no logic impact (indentation, comments, naming) type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels May 10, 2022
@N-Dekker N-Dekker requested review from Leengit and dzenanz May 10, 2022 13:45
@N-Dekker
Copy link
Contributor Author

For the record, this pull request was triggered by the discussion starting at #3339 (comment)

@Leengit
Copy link
Contributor

Leengit commented May 10, 2022

Good idea! I change a few more when I use:

cd git/ITK
find . -type f | egrep '\.[it]?(cc|hh|[ch](\+\+|pp?|xx)?)$' | fgrep -v ThirdParty | \
    xargs sed -r -i \
        -e 's/catch \((const )?/catch (const /g' \
        -e 's/catch \(const \.\.\.\)/catch (...)/g'

Just in case the changes this makes aren't some that you have already considered and rejected ... running that after your commits and then running git diff --name-only lists:
Modules/IO/DCMTK/src/itkDCMTKImageIO.cxx
Modules/IO/PhilipsREC/src/itkPhilipsPAR.cxx
Modules/IO/PhilipsREC/src/itkPhilipsRECImageIO.cxx
Modules/Numerics/FEM/include/itkFEMImageMetricLoad.hxx
Modules/Numerics/FEM/test/itkFEMElement2DTest.cxx
Modules/Numerics/FEM/test/itkFEMElement3DTest.cxx
Modules/Numerics/FEM/test/itkFEMElementTest.cxx
Modules/Numerics/FEM/test/itkFEMExceptionTest.cxx
Modules/Numerics/FEM/test/itkFEMGenerateMeshTest.cxx
Modules/Numerics/FEM/test/itkFEMSolverHyperbolicTest.cxx
Modules/Registration/FEM/test/itkFEMRegistrationFilter2DTest.cxx
Modules/Registration/FEM/test/itkFEMRegistrationFilterTest.cxx
Modules/Registration/FEM/test/itkFEMRegistrationFilterTest2.cxx

Replaced `catch \((\w+) &` with `catch (const $1 &`, using Visual Studio 2019,
Find and Replace, Replace in Files, Use regular expressions.

Following C++ Core Guidelines, April 10, 2022, which says that it is "typically
better" to catch by `const` reference than to catch by non-const reference. At
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#e15-throw-by-value-catch-exceptions-from-a-hierarchy-by-reference

With help from Lee Newberg.
@N-Dekker N-Dekker force-pushed the catch-by-reference-to-const branch from d864f73 to 65ea925 Compare May 10, 2022 15:51
@N-Dekker
Copy link
Contributor Author

Good idea! I change a few more when I use:

cd git/ITK
find . -type f | egrep '\.[it]?(cc|hh|[ch](\+\+|pp?|xx)?)$' | fgrep -v ThirdParty | \
    xargs sed -r -i \
        -e 's/catch \((const )?/catch (const /g' \
        -e 's/catch \(const \.\.\.\)/catch (...)/g'

Thanks @Leengit. Please check the force-push that I just did 😃

@N-Dekker N-Dekker marked this pull request as ready for review May 10, 2022 15:55
@hjmjohnson
Copy link
Member

Love This! Minor request. Can the "xargs command" used be placed in the commit message? Even if it is not perfect, it's a great start when updating all the Remote modules.

@Leengit
Copy link
Contributor

Leengit commented May 10, 2022

Text-wise, I see one more possibility (though perhaps it was rejected for other reasons):

catch (itk::fem::FEMException & e)

Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

👍

@hjmjohnson hjmjohnson merged commit f222a5e into InsightSoftwareConsortium:master May 11, 2022
@N-Dekker
Copy link
Contributor Author

Text-wise, I see one more possibility (though perhaps it was rejected for other reasons):

catch (itk::fem::FEMException & e)

@Leengit Sorry the PR is merged already. Feel free to do a follow-up. But as far as I can see, it's the only case that still wasn't fixed with this PR. With this PR, I only looked for catch \((\w+) & and catch \((\w+::\w) &, so I missed the one case with two colons pairs (::).

@N-Dekker
Copy link
Contributor Author

For the record, this pull request appears to be a follow-up to pull request #1492 "COMP: Exceptions should be caught by constant reference", commit 0cf2baf by Hans (@hjmjohnson), merged on 14 December 2019.

N-Dekker added a commit to N-Dekker/ITKSphinxExamples that referenced this pull request May 16, 2022
Replaced `catch \((\S+) &` & with `catch \(const $1 &`, using Notepad++ v8.3.3,
Find in Files, Search Mode: Regular expression.

Follow-up to ITK pull request InsightSoftwareConsortium/ITK#3419
commit InsightSoftwareConsortium/ITK@f222a5e
(merged on May 11, 2022)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Numerics Issues affecting the Numerics module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module type:Style Style changes: no logic impact (indentation, comments, naming) type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants