STYLE: Enclose the majority of the main function in a try-catch block for Examples/SpatialObjects and Examples/RegistrationITKV4

[krupalbhat]

This commit addresses the review comments from PR #5223 related to issue #2802 . In this commit, examples related to SpatialObjects and RegistrationITKV4 have been enclosed with try catch block.

PR Checklist

• No API changes were made (or the changes have been approved)
• No major design changes were made (or the changes have been approved)

[jhlegarreta]

I do not think including dozens of lines of code, including type aliases, regular assignments, and other statements that we know do not throw ITK exceptions, within try/catch blocks would be a desirable change.

[krupalbhat]

How would you prefer the error handling here? Should I keep try/catch only around the parts that might throw ITK exceptions? Initially, it was like that, but later it was changed to wrap the full main block based on review comments.

[jhlegarreta]

How would you prefer the error handling here? Should I keep try/catch only around the parts that might throw ITK exceptions? Initially, it was like that, but later it was changed to wrap the full main block based on review comments.

I made my comment based on the ITK guidelins (f I recall correctly, this is mentioned explicitly) and many style changes that have reduced the scope of such try/catch blocks. Prior to my comment, I had read this as well #5163 (comment).

And now I see this other one #5223 (comment).

I do not think using a single try/catch block for the entire body is better, but adding one for every ITK operation is not good either. IMO it should be picked where to place them.

[hjmjohnson]

These examples are an important part of the software guide. The extra indentation will cause errors in generating the software guide.

These examples are to demonstrate major ITK design principles, and they are not intended to be robust applications. While the error handling is good programming practice for production applications, they diminish the core demonstration of ITK features.

I appreciate your submission, but I think that the benefits of keeping these examples focused only on ITK features outweighs the benefits of the improved error checking.

[krupalbhat]

Thank you for the clarification. Should I go ahead and close this PR? I also need some guidance—there are still some examples that can be updated to use itk::ReadImage and itk::WriteImage. In those cases, should I add local try-catch blocks for exception handling, or is it better to not add any exception handling?"

[N-Dekker]

Personally I like the direction of the PR. It's in general recommended to prevent exceptions from leaving the main function. Statistic analysis tools even produce a warning for such cases: "Warning: An exception may be thrown in function 'main' which should not throw exceptions" (JetBrains ReSharper) or https://clang.llvm.org/extra/clang-tidy/checks/bugprone/exception-escape.html (Clang-Tidy).

I also think it's a good idea to show how to catch those exceptions in the examples of ITK.

It's fine to me to place the entire body of a main function inside a try-catch block. If the extra indentation feels cumbersome, the inner part of the code might be moved to a helper function. For example:

namespace
{
// Note: ExampleMain might throw an exception!
int
ExampleMain(const int argc, const char * const * const argv)
{
  (The original code from main should go here!)
}
} // namespace

int
main(int argc, char * argv[])
{
  try
  {
    return ExampleMain(argc, argv);
  }
  catch (const itk::ExceptionObject & exceptionObject)
  {
    std::cerr << "ITK exception caught:\n" << exceptionObject << '\n';
  }
  catch (const std::exception & stdException)
  {
    std::cerr << "std exception caught:\n" << stdException.what() << '\n';
  }
  return EXIT_FAILURE;
}

My 2 cents, Niels

[dzenanz]

Generally looks good. Thank you for the effort.

commit a422c27 has an invalid commit subject; the first line must be no longer than 78 characters.

It would be good if you amended the commit to change the commit message according to the above. If not, I can do it for you next week.

[dzenanz]

@hjmjohnson clang-format has enforced the line limit, including reflowing the text here.

[dzenanz]

It is much easier to review this if whitespace changes are hidden.

[krupalbhat]

Generally looks good. Thank you for the effort.

commit a422c27 has an invalid commit subject; the first line must be no longer than 78 characters.

It would be good if you amended the commit to change the commit message according to the above. If not, I can do it for you next week.

Thank you for the clarification. I have amended the commit message.

[jhlegarreta]

I very much dislike the change and agree with Hans #5246 (review). So I cannot approve this change.

[dzenanz]

Can you propose an alternative? Or pick one example most in need of change, and show how to change it?

[jhlegarreta]

Can you propose an alternative? Or pick one example most in need of change, and show how to change it?

The issue #2802 has been previously addressed changing just the concerned block and without requiring to enclose any additional part of the code in try/catchblocks or using ITK_TRY_EXPECT_NO_EXCEPTION to hold the statements, e.g.:
https://github.com/InsightSoftwareConsortium/ITK/pull/2160/files
https://github.com/InsightSoftwareConsortium/ITK/pull/2173/files
https://github.com/InsightSoftwareConsortium/ITK/pull/2457/files
https://github.com/InsightSoftwareConsortium/ITK/pull/3325/files

So again, re #5223 (comment), I do not see the need to enclose parts that will never trigger an exception into a try/catch block.

[hjmjohnson]

FYI: I am not opposed to this set of conmits. I appreciate the community efforts. I'm on the fence about the need to make example code "perfect in all ways".

From my teaching, my tendency is to make examples as focused and simple as possible, even if it is not best-practices complete all around. With that said, I am conflicted about where these examples fall on that continuum of completeness.

I realize that the code is re-flowed, but that does affect the code in the software guide, and may make the code longer (more lines). Perhaps we can "de-indent" the examples in the software guide.

[blowekamp]

WIth optimization (such as registration and deep learning), it can be frustrating when processing runs for a long time and the some error occurs and results are not saved.

In the current change the exception handling is not useful. It may give a more descriptive output? Another utility of the exception handling may be to save the intermediate results?

[dzenanz]

The main benefit of enclosing the entire "main" into a try-catch block is not having to think about what statements (and even constructors) might throw. No matter where the exception occurs, some error message is produced.

For indentation problems, Niels' suggestion seems good. I could not find any indentation settings specific to try-catch block in clang-format. The closest is BraceWrappingBeforeCatch.

[N-Dekker]

Thanks @dzenanz For the sake of completeness, regarding my suggestion at #5246 (comment): Strictly speaking, the catch (const itk::ExceptionObject &) isn't necessary. A general catch (const std::exception &) catches exceptions from ITK as well (because itk::ExceptionObject is derived from std::exception).

[dzenanz]

That is correct, but ITK exception is richer, usually containing the file/line where the error occured.

[krupalbhat]

How should i proceed for now?

[dzenanz]

Another utility of the exception handling may be to save the intermediate results?

Adding that feature would be another step.

For now, do we agree that Niels' suggestion is good enough? I like and use this pattern a lot, see e.g. this montage example.