WIP: Remove non-const overload of `Object::AddObserver`, add `mutable`

[N-Dekker]

Declared m_SubjectImplementation mutable (just like m_MetaDataDictionary), in order to avoid a const_cast<Self *> in the const overload of Object::AddObserver. Removed the non-const overload, as it is semantically just the same as the const overload.

Follow-up to pull request https://github.com/InsightSoftwareConsortium/ITK/pull/3634 commit ebddf08ba028480af3b6ef0c214172e65f6e4f78 "STYLE: Remove const overload of SubjectImplementation::AddObserver"

[N-Dekker]

In general, I'm reluctant to add more mutable keywords, but if it allows removing const_casts and duplicate code, it may be justifiable 😺

[N-Dekker]

Hmmm... PythonTypemapsTest fails on ITK.Linux.Python and ITK.macOS.Python, https://open.cdash.org/test/811897245 says:

Exception caught as expected /Users/runner/work/1/s/Modules/Core/Common/src/itkProcessObject.cxx:1339:
ITK ERROR: MedianImageFilter(0x7fa3157e0900): Input Primary is required but not set.
Traceback (most recent call last):
  File "/Users/runner/work/1/s/Wrapping/Generators/Python/Tests/PythonTypemapsTest.py", line 144, in <module>
    median.AddObserver(itk.DeleteEvent(), exit)
  File "/Users/runner/work/1/s-build/Wrapping/Generators/Python/itk/ITKCommonBasePython.py", line 984, in AddObserver
    if len(args) == 3 and not issubclass(args[2].__class__, itk.Command) and callable(args[2]):
UnboundLocalError: local variable 'args' referenced before assignment
itkTestDriver: Process exited with return value: 1

[dzenanz]

CMake Warning (dev) at /usr/local/share/cmake-3.24/Modules/ExternalProject.cmake:3071 (message):
  The DOWNLOAD_EXTRACT_TIMESTAMP option was not given and policy CMP0135 is
  not set.  The policy's OLD behavior will be used.  When using a URL
  download, the timestamps of extracted files should preferably be that of
  the time of extraction, otherwise code that depends on the extracted
  contents might not be rebuilt if the URL changes.  The OLD behavior
  preserves the timestamps from the archive instead, but this is usually not
  what you want.  Update your project to the NEW behavior or specify the
  DOWNLOAD_EXTRACT_TIMESTAMP option with a value of true to avoid this
  robustness issue.
Call Stack (most recent call first):
  /usr/local/share/cmake-3.24/Modules/ExternalProject.cmake:4167 (_ep_add_download_command)
  Wrapping/Generators/SwigInterface/CMakeLists.txt:142 (ExternalProject_Add)
This warning is for project developers.  Use -Wno-dev to suppress it.

This seems unrelated to this PR. But PythonTypemapsTest still fails.

[N-Dekker]

But PythonTypemapsTest still fails.

Strange! Obviously this rather simple style improvement cannot be merged until this test passed successfully again! 🤔

[dzenanz]

Matt say's "The non-const may be needed in this case." Let's see whether the current builds finish without issue.

[N-Dekker]

Matt say's "The non-const may be needed in this case."

Thanks @dzenanz, but that still makes me wonder why! Normally in C++ a "const" member function may be called on both "const" and non-const instances of a class. It should not be necessary to overload for const and non-const when both overloads do exactly the same thing.

Could it be that there is a Python wrapping, specifically for the non-const overload? I don't know how that would work, my Python wrapping knowledge is still limited.

Let's see whether the current builds finish without issue.

Not very hopeful though. I think my last force-push only fixes the merge conflict.

[blowekamp]

Strange! Obviously this rather simple style improvement cannot be merged until this test passed successfully again!

I would not consider this a "simple style improvement". It is an API change.

[N-Dekker]

I would not consider this a "simple style improvement". It is an API change.

@blowekamp Can you please explain? From an end-user perspective, do you see any change?

[blowekamp]

@N-Dekker A function from the public interface is being remove. From the usability stand point it most likely will not change, but a member function is none the less being remove from the API.

[N-Dekker]

Still wondering about the error message from https://open.cdash.org/test/813223872 saying:

File "/home/vsts/work/1/s-build/Wrapping/Generators/Python/itk/ITKCommonBasePython.py", line 984, in AddObserver if len(args) == 3 and not issubclass(args[2].__class__, itk.Command) and callable(args[2]): UnboundLocalError: local variable 'args' referenced before assignment

Looking at pyBase.i, it says:

https://github.com/InsightSoftwareConsortium/ITK/blob/a7546b667d708836e0abd4aab1442d2daa055546/Wrapping/Generators/Python/PyBase/pyBase.i#L308-L310

So indeed, the definition of args seems to be missing! Where should it be?

[thewtex]

Could it be that there is a Python wrapping, specifically for the non-const overload?

Yes. Python is C -- it does not use all the niceties of C++ sometimes.

@blowekamp is right -- this is an API change.

[N-Dekker]

Yes. Python is C -- it does not use all the niceties of C++ sometimes.

Thanks @thewtex, but sorry, I still don't understand. Does that mean that each and every const member function needs to be overloaded for non-const, in order to support Python wrapping?

[N-Dekker]

Update: I finally start to understand how removing the redundant Object::AddObserver overload affects SWIG! When Object::AddObserver is overloaded as it has been so far (having a redundant non-const overload), SWIG produces a file, "build-directory\Wrapping\Generators\Python\itk\ITKCommonBasePython.py" which has:

def AddObserver(self, *args)

However, when the redundant non-const overload is removed (as I proposed), SWIG produces the following, instead, based on the single remaining AddObserver(const EventObject & event, Command *) member function:

def AddObserver(self, event, arg3)

The %pythonprepend from ITK's pyBase.i assumes that SWIG has an args parameter, even though it is not officially documented by SWIG: https://github.com/InsightSoftwareConsortium/ITK/blob/a7546b667d708836e0abd4aab1442d2daa055546/Wrapping/Generators/Python/PyBase/pyBase.i#L308-L310

According to James, at https://stackoverflow.com/questions/24982790/how-to-use-pythonappend-and-pythonprepend-to-access-argument-and-return-values: "Unfortunately, there seems to be no official support for accessing argument and return values. This approach might break your code in future swig releases."

Interestingly, if we would have chosen to have a named parameter for the Command * argument in the C++ header file, this would also appear in the SWIG wrapping:

AddObserver(const EventObject & event, Command * command) const;

would SWIG to:

def AddObserver(self, event, command)

which is clearer that either args or arg3. The %pythonprepend code could be very much simplified if it could refer to the command parameter by its name "command", rather than by args[2]. Maybe something to look at after the release of ITK 5.3 😃

[dzenanz]

Closing in favor of #3669.