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.

Sign up for GitHub

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

Jump to bottom

Improve ITKReader, testITKio and testITKReader #1379

Merged
merged 14 commits into from
Jan 26, 2019
Merged

Improve ITKReader, testITKio and testITKReader #1379

merged 14 commits into from
Jan 26, 2019

Conversation

BorisMansencal
Copy link
Contributor

@BorisMansencal BorisMansencal commented Jan 14, 2019 •
edited by dcoeurjo
Loading

PR Description

Currently, when DGtal is configured with ITK, testITKio is failing.

This PR:

  • add UINT and INT types handling to ITKReader to make testITKio pass
  • add ULONG and LONG type handling to ITKReader and corresponding tests (in testITKio)
  • fix ITKReader crash on non existing files and add corresponding test (in testITKReader)
  • clean code (in particular remove some dead code) in ITKReader, testITKio and testITKReader

It compiles with gcc (8.2.1) and clang (7.0) and pass tests.

In Debug mode, gcc 8.2.1 produces some warnings. Some are specific to ITK. Another is due to the "implicit fallthrough" in the switch case line 71 of ITKReader.ih. I don't know if it was the expected behavior. If it is, a comment with "Falls through" (for example) should be added to silence gcc7 implicit-fallthrough warning (see https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wimplicit-fallthrough ).

Checklist

  • Unit-test of your feature with Catch.
  • Doxygen documentation of the code completed (classes, methods, types, members...)
  • Documentation module page added or updated.
  • New entry in the ChangeLog.md added.
  • No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).
  • All continuous integration tests pass (Travis & appveyor)

@BorisMansencal
Add missing types in switch case to pass testITKio
45c5b90
@BorisMansencal
nb and nbok uses were reversed
98dcd7d
@BorisMansencal
Add long and unsigned long tests
4d914e9 
long and unsigned long images are saved as LONLLONG and ULONGLONG by itkMetaImageIO, so handle these types in ITKReader.
@BorisMansencal
remove dead code
3604af5
@BorisMansencal
Don't crash on non existent file
84a8768 
Correctly handle non existent image file in ITKReader and add corresponding test.
@BorisMansencal
Remove useless allocation.
f58dad0 
No need to initialize itk_image variable as it will be overwritten.
@BorisMansencal
remove dead code
210b094
@BorisMansencal
Reserve memory of vector
b96ac0e
@BorisMansencal
update ChangeLog for Pull Request
d046924
@BorisMansencal
update ChangeLog.md
e4117ea
@BorisMansencal
Copy link
Contributor Author

It seems that the build stalled on OSX.
Is there a way to launch this build again ?

FYI, this code compiles and tests pass on macOS High Sierra (10.13.6) with "Apple LLVM version 8.0.0 (clang-800.0.42.1)".

@rolanddenis
Copy link
Member

You can log in to Travis using your Github credentials and then restart your jobs. The job on OSX seems to pass now :octocat:

@dcoeurjo dcoeurjo added this to Inbox in Issue triage via automation Jan 15, 2019
@dcoeurjo dcoeurjo added this to the 1.0 milestone Jan 15, 2019
@BorisMansencal
Silence pedantic warnings on ITK headers
36863f3 
In Debug mode, -Wpedantic of gcc 8.2.1 warns about extra ";" after some ITK macros.
@BorisMansencal
Silence clang documentation warning on ITK headers
d0686c2
@BorisMansencal
Explicit message for falltrhough warning
17e8f92 
In Debug, gcc 8.2.1 [-Wimplicit-fallthrough] warns about the implicit fallthrough in the switch case. Add a more explicit message and a comment to silence the warning.
@BorisMansencal
Copy link
Contributor Author

I added some small corrections to debug some warnings when compiled in Debug mode (with gcc 8.2.1 or clang 7.0.0.
I also added a more explicit message and a "//fallthrough" comment that silence the "implicit fallthrough" warning.

@BorisMansencal BorisMansencal mentioned this pull request Jan 17, 2019
Merged
6 tasks
@dcoeurjo
Copy link
Member

Thanks for the PR !

dcoeurjo
dcoeurjo reviewed Jan 19, 2019
View reviewed changes
Copy link
Member

@dcoeurjo dcoeurjo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I've just few minor coding style issues.

src/DGtal/io/readers/ITKReader.ih Outdated Show resolved Hide resolved
tests/io/readers/testITKReader.cpp Outdated Show resolved Hide resolved
tests/io/readers/testITKReader.cpp Outdated Show resolved Hide resolved
tests/io/readers/testITKReader.cpp Show resolved Hide resolved
@dcoeurjo
Copy link
Member

All good. Ma y thanks.

@dcoeurjo dcoeurjo merged commit 02bd69b into DGtal-team:master Jan 26, 2019
Issue triage automation moved this from Inbox to Done Jan 26, 2019
@BorisMansencal BorisMansencal deleted the ITKio1 branch February 5, 2019 09:35
@BorisMansencal BorisMansencal mentioned this pull request Feb 14, 2019
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcoeurjo dcoeurjo dcoeurjo left review comments

Assignees
No one assigned
Labels
Build Image waiting author feedbacks
Projects
Issue triage
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@BorisMansencal @rolanddenis @dcoeurjo