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

BUG: Remove SpatialObjectProperty's writable string methods from SWIG #4843

Merged
merged 1 commit into from
Sep 11, 2024
Merged

BUG: Remove SpatialObjectProperty's writable string methods from SWIG #4843

merged 1 commit into from
Sep 11, 2024

Conversation

blowekamp
Copy link
Member

SWIG's binding for writable std::string objects is a "std::string *" which is not implemented.

closes #4779

replaces #4776

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)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

@blowekamp blowekamp requested review from aylward and dzenanz September 9, 2024 18:27
@github-actions github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances area:Core Issues affecting the Core module labels Sep 9, 2024
@blowekamp
Copy link
Member Author

Seems to work:
image

dzenanz
dzenanz reviewed Sep 9, 2024
View reviewed changes
Copy link
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.

LGTM, but I defer to @aylward

blowekamp
blowekamp commented Sep 11, 2024
View reviewed changes
Modules/Core/SpatialObjects/include/itkSpatialObjectProperty.h Outdated
@@ -123,15 +125,13 @@ class ITKSpatialObjects_EXPORT SpatialObjectProperty
return value;
}


#if !defined(SWIG) && !defined(ITK_WRAPPING_PARSER)
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should only be dependent on ITK_WRAPPING_PARSER

@blowekamp
BUG: Remove SpatialObjectProperty's writable string methods from SWIG
80be78b 
SWIG's binding for writable std::string objects is a "std::string *"
which is not implemented.
@blowekamp blowekamp force-pushed the fix_spatial_object_property_string branch from f9e557e to 80be78b Compare September 11, 2024 13:13
@blowekamp
Copy link
Member Author

@aylward @thewtex Does this fix need to be applied to the release branch too?

@blowekamp blowekamp mentioned this pull request Sep 11, 2024
Closed
@blowekamp blowekamp merged commit 10ad9a6 into InsightSoftwareConsortium:master Sep 11, 2024
13 checks passed
@thewtex
Copy link
Member

thewtex commented Sep 11, 2024

@blowekamp thanks for following up with a patch on this!

@aylward @thewtex Does this fix need to be applied to the release branch too?

Yes, done! efe773f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzenanz dzenanz dzenanz left review comments

@aylward aylward aylward approved these changes

Assignees
No one assigned
Labels
area:Core Issues affecting the Core module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrapping of member functions that are passed and/or return std:string goes wonky
4 participants
@blowekamp @thewtex @aylward @dzenanz