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

COMP: Add export specification to ITKSpatialObjects #759

Merged
merged 1 commit into from
Apr 24, 2019
Merged

COMP: Add export specification to ITKSpatialObjects #759

merged 1 commit into from
Apr 24, 2019

Conversation

thewtex
Copy link
Member

@thewtex thewtex commented Apr 24, 2019

Closes #743

N-Dekker
N-Dekker approved these changes Apr 24, 2019
View reviewed changes
Copy link
Contributor

@N-Dekker N-Dekker left a comment

Choose a reason for hiding this comment

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

Thanks, @thewtex !

fbudin69500
fbudin69500 reviewed Apr 24, 2019
View reviewed changes
Copy link
Contributor

@fbudin69500 fbudin69500 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

These changes look good but I think you missed a couple others:

Although for the second one I am not 100% sure it needs it as it does not seem consistent across the other factory classes in ITK: ObjectFactory does not have any export attribute but NiftiImageIOFactory does have an export attribute.

@thewtex
Copy link
Member Author

thewtex commented Apr 24, 2019

Thanks for the review.

MetaImageMaskConverter, SpatialObjectFactory, and ObjectFactory are header-only, so they should not have the export specification. NiftiImageIOFactory looks like it has the export specification.

@fbudin69500
Copy link
Contributor

You are correct, those two other files that I mentioned are header only. Should we come up with a script (used in a test) that verifies the export symbols?

@thewtex
Copy link
Member Author

thewtex commented Apr 24, 2019

We should have errors on the dashboard if export specifications are not correct. It was not an issue with the ITKSpatialObjects module because ENABLED_SHARED was not set on the module -- it was always being built statically.

@thewtex thewtex merged commit 8c3e6ca into InsightSoftwareConsortium:master Apr 24, 2019
@thewtex thewtex deleted the spatial-object-exports branch April 24, 2019 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fbudin69500 fbudin69500 fbudin69500 approved these changes

@N-Dekker N-Dekker N-Dekker approved these changes

@dzenanz dzenanz Awaiting requested review from dzenanz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Potential link errors classes of SpatialObjects library
3 participants
@thewtex @fbudin69500 @N-Dekker