Use `unique_ptr` internally in the implementation of `itk::Object` #3620

N-Dekker · 2022-09-15T14:18:24Z

Declared the observers of itk::Object as list of Observer objects (instead of a list of raw pointers).
Declared both m_SubjectImplementation and m_MetaDataDictionary as an std::unique_ptr.
Declared Observer::m_Event as unique_ptr

N-Dekker · 2022-09-15T14:51:33Z

Any idea what the ITK.macOS.Python failure is about? https://open.cdash.org/viewBuildError.php?buildid=8158841 says:

CMake Error at KWStyle-prefix/src/KWStyle-stamp/KWStyle-download-Release.cmake:49 (message):

Command failed: 1

'/usr/local/Cellar/cmake/3.24.1/bin/cmake' '-P' '/.../s-build/KWStyle-prefix/tmp/KWStyle-gitclone.cmake'

See also

/.../s-build/KWStyle-prefix/src/KWStyle-stamp/KWStyle-download-*.log

dzenanz · 2022-09-15T14:57:20Z

Looking at Azure log:

CMake Deprecation Warning at /Users/runner/work/1/ITK-dashboard/itk_common.cmake:96 (cmake_minimum_required):
  Compatibility with CMake < 2.8.12 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.
Call Stack (most recent call first):
  /Users/runner/work/1/ITK-dashboard/azure_dashboard.cmake:82 (include)
  /Users/runner/work/1/ITK-dashboard/dashboard.cmake:12 (include)

and then later on

[527/5723] Building C object Modules/ThirdParty/VNL/src/vxl/v3p/netlib/CMakeFiles/itkv3p_netlib.dir/lapack/complex16/zlascl.c.o
[528/5723] Performing download step (git clone) for 'KWStyle'
FAILED: KWStyle-prefix/src/KWStyle-stamp/KWStyle-download /Users/runner/work/1/s-build/KWStyle-prefix/src/KWStyle-stamp/KWStyle-download 
cd /Users/runner/work/1/s-build && /usr/local/Cellar/cmake/3.24.1/bin/cmake -P /Users/runner/work/1/s-build/KWStyle-prefix/src/KWStyle-stamp/KWStyle-download-Release.cmake && /usr/local/Cellar/cmake/3.24.1/bin/cmake -E touch /Users/runner/work/1/s-build/KWStyle-prefix/src/KWStyle-stamp/KWStyle-download
CMake Error at KWStyle-prefix/src/KWStyle-stamp/KWStyle-download-Release.cmake:49 (message):
  Command failed: 1

   '/usr/local/Cellar/cmake/3.24.1/bin/cmake' '-P' '/Users/runner/work/1/s-build/KWStyle-prefix/tmp/KWStyle-gitclone.cmake'

  See also

    /Users/runner/work/1/s-build/KWStyle-prefix/src/KWStyle-stamp/KWStyle-download-*.log


[529/5723] Building C object Modules/ThirdParty/VNL/src/vxl/v3p/netlib/CMakeFiles/itkv3p_netlib.dir/lapack/complex16/zung2r.c.o
[530/5723] Building C object Modules/ThirdParty/VNL/src/vxl/v3p/netlib/CMakeFiles/itkv3p_netlib.dir/lapack/complex16/ztrevc.c.o
[531/5723] Building C object Modules/ThirdParty/VNL/src/vxl/v3p/netlib/CMakeFiles/itkv3p_netlib.dir/lapack/complex16/zlatrs.c.o
[532/5723] Building C object Modules/ThirdParty/VNL/src/vxl/v3p/netlib/CMakeFiles/itkv3p_netlib.dir/lapack/complex16/zlarfx.c.o
ninja: build stopped: subcommand failed.
Command exited with the value: 1
MakeCommand:/usr/local/Cellar/cmake/3.24.1/bin/cmake --build . --config "Release"
   1 Compiler errors
   1 Compiler warnings
Error(s) when building project
SetCTestConfiguration:BuildDirectory:/Users/runner/work/1/s-build
SetCTestConfiguration:SourceDirectory:/Users/runner/work/1/s
Test project /Users/runner/work/1/s-build
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 1
        Start   1: KWStyleExamplesTest
Could not find executable /Users/runner/work/1/s-build/KWStyle-build/KWStyle
Looked in the following places:
/Users/runner/work/1/s-build/KWStyle-build/KWStyle
/Users/runner/work/1/s-build/KWStyle-build/KWStyle
/Users/runner/work/1/s-build/KWStyle-build/Release/KWStyle
/Users/runner/work/1/s-build/KWStyle-build/Release/KWStyle
Release//Users/runner/work/1/s-build/KWStyle-build/KWStyle
Release//Users/runner/work/1/s-build/KWStyle-build/KWStyle
Users/runner/work/1/s-build/KWStyle-build/KWStyle
Users/runner/work/1/s-build/KWStyle-build/KWStyle
Users/runner/work/1/s-build/KWStyle-build/Release/KWStyle
Users/runner/work/1/s-build/KWStyle-build/Release/KWStyle
Release/Users/runner/work/1/s-build/KWStyle-build/KWStyle
Release/Users/runner/work/1/s-build/KWStyle-build/KWStyle

1: Test command:  "-xml" "/Users/runner/work/1/s/Utilities/KWStyle/ITK.kws.xml" "-v" "-o" "/Users/runner/work/1/s/Utilities/KWStyle/ITKOverwrite.txt" "-D" "/Users/runner/work/1/s-build/Utilities/KWStyle/ITKExamplesFiles.txt" "-gcc"
1: Working Directory: /Users/runner/work/1/s
Unable to find executable: /Users/runner/work/1/s-build/KWStyle-build/KWStyle
  1/291 Test   #1: KWStyleExamplesTest .....................................................***Not Run   0.00 sec
test 2
        Start   2: vcl_test_algorithm

dzenanz · 2022-09-15T15:01:12Z

I don't know why would git clone from GitHub keep failing. https://github.com/InsightSoftwareConsortium/ITK/blob/v5.3rc04/Utilities/KWStyle/BuildKWStyle.cmake#L25

N-Dekker · 2022-09-16T10:22:07Z

/azp run ITK.macOS.Python

dzenanz · 2022-09-16T15:36:10Z

Modules/Core/Common/src/itkObject.cxx

  template <typename TObject>
  void
-  InvokeEventRecursion(const EventObject & event, TObject * self, std::list<Observer *>::reverse_iterator & i);
+  InvokeEventRecursion(const EventObject & event, TObject * self, ObserverListType::reverse_iterator & i);


Will this change in the public interface cause backwards compatibility issues?

Good question, thanks @dzenanz but no, I don't see any backwards compatibility issue here. InvokeEventRecursion is a protected member function of a "hidden" class (ITKCommon_HIDDEN) named "SubjectImplementation", which is defined inside itkObject.cxx. So InvokeEventRecursion is also declared and defined in itkObject.cxx, and it isn't being called outside itkObject.cxx. So it's really just an implementation detail. As far as I can see.

blowekamp

I am rather uncertain if the recursive function still works as intended as the list is the sole owner.

blowekamp · 2022-09-16T16:29:27Z

Modules/Core/Common/src/itkObject.cxx

@@ -219,7 +208,7 @@ SubjectImplementation::InvokeEventRecursion(const EventObject &
  {

    // save observer
-    const Observer * o = *i;
+    const auto & o = *i;


I'm not sure what the type is here? Reference to the unique pointer in the list?

The point of this pattern is to save the pointer to the object, in case is deleted/modified. Since the list still has the sole ownership I don't believe this works as intended any more. See the prose above.

Perhaps this should be maintained as a raw pointer, and the below "find" be modified to compare raw pointer of m_Observer items to this save raw pointer?

Is InvokeEventRecursion exercised in current tests? If not, now would be a good time to cover it by test(s).

There was a reasonable test added for this case: f7307e2 It may be that valgrind will be required to detect the invalid memory access.

[deleted.... I'll be back!]

Perhaps this should be maintained as a raw pointer, and the below "find" be modified to compare raw pointer of m_Observer items to this save raw pointer?

Thanks @blowekamp This comment of yours inspired me to submit pull request #3629: "BUG: Object::InvokeEvent should not use a dangling Observer pointer"! Please check!

I think this PR (#3620) and PR #3629 can be processed in either order, but it's probably easiest now to first process PR #3629.

blowekamp

With #3629 merged this should be good to go.

N-Dekker · 2022-09-19T15:17:17Z

With #3629 merged this should be good to go.

Thanks @blowekamp, I'm actually preparing a revision! It appears that because we just abandoned Observer pointer comparison in favor of tag comparison, the list may now simply contain the observers themselves, rather than their pointers! Which further simplifies their memory management! 😃

blowekamp · 2022-09-19T15:28:19Z

@N-Dekker If you are refactoring the whole thing, the recursive implementation with the search at each level is O(N**2) complexity. We should be able to use a better data structure to reduce the complexity, and improve the efficiency of the is the observer still there.

Does the order of the observers are visited matter? Maybe.

N-Dekker · 2022-09-19T15:42:50Z

If you are refactoring the whole thing, the recursive implementation with the search at each level is O(N**2) complexity. We should be able to use a better data structure to reduce the complexity, and improve the efficiency of the is the observer still there.

Thanks @blowekamp but my focus is still on trying to avoid manual memory management (new/delete). A change from std::list<Observer*> to std::list<Observer> appears low hanging fruit, in that context. Switching to a different data structure sounds more challenging to me. Theoretically, other data structures may still be faster, but it really depends on the use case. How many observers does an observed object "usually" have? For a small number of observers, O(N) may not be so much better than O(1).

N-Dekker · 2022-09-19T19:29:25Z

Ready to be merged.... although PR #3634 "STYLE: Remove const overload of SubjectImplementation::AddObserver" could also be merged first! (PR #3634 removes a few lines of code that are actually adjusted by this PR! These adjustments are no longer necessary when PR #3634 is merged beforehand.)

dzenanz · 2022-09-19T19:46:23Z

I merged #3634. Now this branch has merge conflicts.

Declared both `m_SubjectImplementation` and `m_MetaDataDictionary` as an `std::unique_ptr`.

The original code technically allowed an `Observer` (from the implementation of `itk::Object`) to be copied, causing its destructor to possibly be called twice, trying to do `delete m_Event` twice on the very same event object. Which would have undefined behavior, possibly causing a crash. This commit solves this potential problem by declaring m_Event as a `unique_ptr`, and removing the user-defined `Observer` destructor (leaving it "implicitly defaulted", following the Rule of Zero). Thereby it also removes the `virtual` keyword from the destructor, which is OK, because `Observer` is not a base class.

Instead of using raw pointers. Also "defaulted" the destructor of `SubjectImplementation`.

N-Dekker · 2022-09-19T20:12:43Z

I merged #3634. Now this branch has merge conflicts.

Thanks @dzenanz The conflicts should be fixed with the rebase master + force-push that I just did. 😃

N-Dekker · 2022-09-20T12:31:41Z

OK, I think this one is ready to be merged, again! 😄 I was trying to sneak in another partially overlapping pull request, PR #3636 ("STYLE: Remove non-const overload of Object::AddObserver, add mutable") but it got stuck somehow, and it shouldn't stop this one (#3620) from getting merged anyway!

github-actions bot added the area:Core Issues affecting the Core module label Sep 15, 2022

N-Dekker force-pushed the Object-unique_ptr branch from 3074254 to c8376b2 Compare September 15, 2022 14:29

N-Dekker marked this pull request as ready for review September 15, 2022 20:35

N-Dekker requested review from blowekamp and dzenanz September 16, 2022 15:23

dzenanz reviewed Sep 16, 2022

View reviewed changes

blowekamp requested changes Sep 16, 2022

View reviewed changes

N-Dekker marked this pull request as draft September 17, 2022 10:47

dzenanz approved these changes Sep 19, 2022

View reviewed changes

blowekamp approved these changes Sep 19, 2022

View reviewed changes

N-Dekker force-pushed the Object-unique_ptr branch 2 times, most recently from cfeb38d to 5231f32 Compare September 19, 2022 15:20

dzenanz approved these changes Sep 19, 2022

View reviewed changes

N-Dekker marked this pull request as ready for review September 19, 2022 19:29

N-Dekker added 3 commits September 19, 2022 22:07

STYLE: Declare two private data members of itk::Object as unique_ptr

bea8dcb

Declared both `m_SubjectImplementation` and `m_MetaDataDictionary` as an `std::unique_ptr`.

STYLE: Declare observers of itk::Object as list of Observer objects

ad87937

Instead of using raw pointers. Also "defaulted" the destructor of `SubjectImplementation`.

N-Dekker force-pushed the Object-unique_ptr branch from 5231f32 to ad87937 Compare September 19, 2022 20:10

dzenanz approved these changes Sep 19, 2022

View reviewed changes

dzenanz merged commit 171fb2b into InsightSoftwareConsortium:master Sep 20, 2022

maxaehle mentioned this pull request Sep 21, 2022

Compilation fails with GCC 8.4. #3643

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `unique_ptr` internally in the implementation of `itk::Object` #3620

Use `unique_ptr` internally in the implementation of `itk::Object` #3620

N-Dekker commented Sep 15, 2022 •

edited

Loading

N-Dekker commented Sep 15, 2022

dzenanz commented Sep 15, 2022

dzenanz commented Sep 15, 2022

N-Dekker commented Sep 16, 2022

dzenanz Sep 16, 2022

N-Dekker Sep 16, 2022

blowekamp left a comment

blowekamp Sep 16, 2022 •

edited

Loading

blowekamp Sep 16, 2022

dzenanz Sep 16, 2022

blowekamp Sep 16, 2022

N-Dekker Sep 17, 2022 •

edited

Loading

N-Dekker Sep 17, 2022

blowekamp left a comment

N-Dekker commented Sep 19, 2022

blowekamp commented Sep 19, 2022

N-Dekker commented Sep 19, 2022

N-Dekker commented Sep 19, 2022 •

edited

Loading

dzenanz commented Sep 19, 2022

N-Dekker commented Sep 19, 2022

N-Dekker commented Sep 20, 2022

Use unique_ptr internally in the implementation of itk::Object #3620

Use unique_ptr internally in the implementation of itk::Object #3620

Conversation

N-Dekker commented Sep 15, 2022 • edited Loading

N-Dekker commented Sep 15, 2022

dzenanz commented Sep 15, 2022

dzenanz commented Sep 15, 2022

N-Dekker commented Sep 16, 2022

dzenanz Sep 16, 2022

Choose a reason for hiding this comment

N-Dekker Sep 16, 2022

Choose a reason for hiding this comment

blowekamp left a comment

Choose a reason for hiding this comment

blowekamp Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

blowekamp Sep 16, 2022

Choose a reason for hiding this comment

dzenanz Sep 16, 2022

Choose a reason for hiding this comment

blowekamp Sep 16, 2022

Choose a reason for hiding this comment

N-Dekker Sep 17, 2022 • edited Loading

Choose a reason for hiding this comment

N-Dekker Sep 17, 2022

Choose a reason for hiding this comment

blowekamp left a comment

Choose a reason for hiding this comment

N-Dekker commented Sep 19, 2022

blowekamp commented Sep 19, 2022

N-Dekker commented Sep 19, 2022

N-Dekker commented Sep 19, 2022 • edited Loading

dzenanz commented Sep 19, 2022

N-Dekker commented Sep 19, 2022

N-Dekker commented Sep 20, 2022

Use `unique_ptr` internally in the implementation of `itk::Object` #3620

Use `unique_ptr` internally in the implementation of `itk::Object` #3620

N-Dekker commented Sep 15, 2022 •

edited

Loading

blowekamp Sep 16, 2022 •

edited

Loading

N-Dekker Sep 17, 2022 •

edited

Loading

N-Dekker commented Sep 19, 2022 •

edited

Loading