Wrapping of member functions that are passed and/or return std:string goes wonky

[aylward]

The issues are demonstrated in #4776

1. SWIG seems to be converting some member functions that are passed two std::strings as args to new versions of functions that returns a std::string * (rather than returning a void as defined by the C++ code).

This documentation, though old, might be relevant...
https://www.swig.org/Doc1.3/Arguments.html#Arguments_nn2

That documented feature would cause a function in C++ that returns a void to wrap to a python function that returns a value - hence causing the SetTagValueString to appear to return a value in python.

2. Additionally, SWIG converts return by value to return by pointer:
   https://www.swig.org/Doc1.3/SWIG.html#SWIG_nn23

So, that is where the std::string * references are coming from.

Summary

I suspect there is a bug in our SWIG wrapping specification for these reasons and because new SWIG should handled std::string fine and these wonkynesses shouldn't be happening.

[N-Dekker]

Just wondering, should there be a %include "std_string.i" somewhere, that might be missing? Looking at https://www.swig.org/Doc2.0/Library.html#Library_std_string

[thewtex]

I think we generally want to avoid the use of std::string * in favor of std::string and std::string_view.

[N-Dekker]

I would like to see a minimal example where f1(const std::string&) would "swig properly", while f2(const std::string&, const std::string&) would go wrong. Actually I would like to try it myself, but not today anymore 🤷

[blowekamp]

I tired compiling with wrapping enabled and ran into issue with the precompiled binaries on Mac ARM:
CastXML/CastXMLSuperbuild#62

[blowekamp]

@aylward In itkSpatialObjectProperty.i I see the following:

%apply (std::string& INOUT) { std::string & value};

Which apply some "funky" input output with then string reference parameter is "value".

This likely is generated from the following code block:

ITK/Wrapping/Generators/SwigInterface/igenerator.py

Lines 1597 to 1611 in e93fa19

	def generate_applyfile(self):
	# When a std::string is passed by reference, we need to add the %apply
	# line with the argument name, and the INOUT command.
	# Use a set() to remove duplicates, this will work event if we got
	# multiple functions with the same argument name in the same .i file
	# (swig should take care of it).
	apply_file_names = set(self.apply_file_names)
	# Apply file, for passing std::string as reference in methods
	apply_file = StringIO()
	for name in apply_file_names:
	apply_file.write(
	"%apply (std::string& INOUT) { std::string & " + name + "};\n"
	)
	apply_file.write("\n\n")
	return apply_file

I conjecture the following line is causing this additional "apply" to corrupt the behavior of the other "std::string" parameters:

 GetTagStringValue(const std::string & tag, std::string & value) const;

If this signature is needed, name the parameter something like "inout_value".

Additionally I recommend return values be "std::string" and input arguments be "const std::string &". There is no current wrapping enabled/tested for "string_value".

[aylward]

Outstanding! I will create a PR to start testing this.

Since we're moving towards ITK 6.0, I will break backward compatibility in ITKSpatialObjects, and update its API to use Get/SetMacros and appropriate variable types (e.g., no double * variables). I'll follow you suggestion of return by value and pass by const reference.

Thanks!!!

[N-Dekker]

In itkSpatialObjectProperty.i I see the following:

%apply (std::string& INOUT) { std::string & value};

I'm glad to see you found the source of this issue, @blowekamp This commit appears to have introduced this behavior: 32df266 "ENH: Allow to use methods which pass std::string by reference from python", Michka Popoff (@iMichka), Jun 28, 2014. Saying:

See ITK-3285.

Adding the following line for each argument in the .i files lets swig know what to do
with std::string when passed by reference:
%apply (std::string& INOUT) { std::string & variableName )

See the test for the syntax in python.

Is the old bug report ITK-3285 still available somewhere? Given that the bug report (ITK-3285) and the commit are both more than 10 years old, is this %apply thing still necessary nowadays?

[dzenanz]

@hjmjohnson @thewtex is the old ITK JIRA still available somewhere?

[N-Dekker]

is the old ITK JIRA still available somewhere

Found it! https://insightsoftwareconsortium.atlassian.net/browse/ITK-3285 "Allow passing std::string by reference to functions", June 27, 2014.

Ironically, it seems like this commit effectively disallows passing std::string by reference! (As it triggered Stephen to change pass by reference to pass by value.) But I guess I just don't understand it well enough!