Replace Modified() call in itk::Object, remove Modified() call from BSplineDeformableTransform

[N-Dekker]

• Removed the Modified() call from the default-constructor of BSplineDeformableTransform, as follow-up to pull request STYLE: Remove SetFixedParameters... functions from BSplineBaseTransform #6854
• Replace the Modified() call with m_MTime.Modified() in the default-constructor of itk::Object. For an Object that is still "under construction", it appears overdone to call this->Modified(), because it does not yet have any observer to listen to its ModifiedEvent. Moreover, it appears preferable to avoid calling virtual functions within a constructor: C++ Core Guidelines, Don’t call virtual functions in constructors and destructors.

---

For the record Object::Modified() is defined here:

ITK/Modules/Core/Common/src/itkObject.cxx

Lines 345 to 353 in fd3b0b4

	/**
	* Make sure this object's modified time is greater than all others.
	*/
	void
	Object::Modified() const
	{
	m_MTime.Modified();
	InvokeEvent(ModifiedEvent());
	}

It appears to have overrides at:

Modules/Core/ImageAdaptors/include/itkImageAdaptor.h
Modules/Filtering/ImageIntensity/include/itkNormalizeImageFilter.h
Modules/Filtering/MathematicalMorphology/include/itkGrayscaleDilateImageFilter.h
Modules/Filtering/MathematicalMorphology/include/itkGrayscaleErodeImageFilter.h
Modules/Filtering/MathematicalMorphology/include/itkGrayscaleMorphologicalClosingImageFilter.h
Modules/Filtering/MathematicalMorphology/include/itkGrayscaleMorphologicalOpeningImageFilter.h
Modules/Filtering/MathematicalMorphology/include/itkMorphologicalGradientImageFilter.h
Modules/Filtering/ParabolicMorphology/include/itkBinaryDilateParabolicImageFilter.h
Modules/Filtering/ParabolicMorphology/include/itkBinaryErodeParabolicImageFilter.h
Modules/Filtering/ParabolicMorphology/include/itkMorphologicalDistanceTransformImageFilter.h
Modules/Filtering/ParabolicMorphology/include/itkParabolicOpenCloseSafeBorderImageFilter.h
Modules/Nonunit/Review/include/itkMiniPipelineSeparableImageFilter.h

[dzenanz]

Change is small, and reasoning makes sense. CI can totally sink it, though 😄

[greptile-apps]

Greptile Summary

Summary

• Initializes object modification timestamps directly during construction and removes a redundant B-spline construction-time modification call.
• No actionable issues were identified.

T-Rex validation blocked

• Runtime compilation could not run because the CMake-generated itkConfigure.h header and the CMake/Ninja toolchain are unavailable.

Confidence Score: 5/5

No blocking issues were identified; the change is safe to merge.

The review identified no actionable findings. The attempted before-and-after construction check was blocked equally in both revisions by missing generated build configuration.

Files Needing Attention: No files require changes. Runtime construction behavior can be rechecked once a configured ITK build environment is available.

T-Rex Logs

What T-Rex did

• I compared construction-time modifications between the pre-change and updated revisions and ran the narrow itkObject.cxx compilation check.
• I observed that both checks halted at the same missing itkConfigure.h header, so runtime construction behavior could not be observed.
• I authored and executed the pr6855-construction-check.sh script, capturing before, after, and runner outputs.
• I documented that the construction-check run produced no actionable findings.

_{Ran code and verified through T-Rex}

_{Reviews (1): Last reviewed commit: "STYLE: Replace Modified() with m_MTime.M..." | Re-trigger Greptile}

[N-Dekker]

Change is small

@dzenanz Indeed 😄 In fact I was just bothered by the unnecessary this->Modified() call in BSplineDeformableTransform, especially because it was introduced with my own pull request (#6854)! But then I thought I could make the PR more general, by removing all this->Modified() calls from default-constructors. Fortunately I just found one more case 🤷

[blowekamp]

This change is probably fine but I don't agree with this comment:

For an Object that is still "under construction", it appears overdone to call this->Modified(), because it does not yet have any observer to listen to its ModifiedEvent.

The thing to consider here is not the event's but the expected consistency of the MTime for a the BSplineTransform object. This class contains other classes. Depending on how the object is expected to operate it may be expected that the MTime of the object >= than any object it owns. Sometimes for a class the GetMTime can return the max off all object it owns.

I don't we are that particular or consistent with the MTime is many object, and this point may not be relevant, but I believe considering the MTime book keeping is more important than the Event side effect.

[N-Dekker]

Thanks for your reply @blowekamp The pull request text may be a bit informal, and indeed, I did not take into consideration that the MTime of an object might in some cases need to be greater than the MTime of its sub-objects. Are the commit messages still OK to you?

Before my pull request #6854, the default-constructor of BSplineDeformableTransform indirectly called this->Modified(), via BSplineBaseTransform::SetFixedParametersFromTransformDomainInformation():

ITK/Modules/Core/Transform/include/itkBSplineDeformableTransform.hxx

Lines 65 to 67 in dbab3e0

	this->SetFixedParametersFromTransformDomainInformation();
	}

BSplineBaseTransform::SetFixedParametersFromTransformDomainInformation() called this->Modified(), here:

ITK/Modules/Core/Transform/include/itkBSplineBaseTransform.hxx

Lines 110 to 112 in dbab3e0

	this->Modified();
	}

So I think that the default-constructor of BSplineDeformableTransform did not intentionally do the extra this->Modified() (which was already done by its common base class itk::Object). It just happened, because of the SetFixedParameters... call.

[blowekamp]

Thanks for your reply @blowekamp The pull request text may be a bit informal, and indeed, I did not take into consideration that the MTime of an object might in some cases need to be greater than the MTime of its sub-objects. Are the commit messages still OK to you?

There is the potential that is could confuse further people and AI looking at this code and understanding the MTime system. I am not going to give a blocking review due to this.

[dzenanz]

Maybe update MTime at the end of the BSpline constructor? Would that be better Brad?

[blowekamp]

Please remove from the commit message:
" - Object::Modified() calls InvokeEvent(ModifiedEvent()), which is useless
when an Object is still "under construction", as it does not yet have any
observer.
"
And the similar in the PR description.

[N-Dekker]

Please remove from the commit message:
" - Object::Modified() calls InvokeEvent(ModifiedEvent()), which is useless
when an Object is still "under construction", as it does not yet have any
observer.
"
And the similar in the PR description.

@blowekamp Thanks but can you please explain why? During the default-construction of an itk::Object, it cannot yet have any observers, right?

Otherwise, can you please present a testable example where InvokeEvent is already useful for an itk::Object that is still under construction?

---

You may consider this example:

class MyObject : public itk::Object
{
public:
  ITK_DISALLOW_COPY_AND_MOVE(MyObject);
  using Self = MyObject;
  using Superclass = Object;
  using Pointer = itk::SmartPointer<Self>;
  using ConstPointer = itk::SmartPointer<const Self>;
  itkOverrideGetNameOfClassMacro(MyObject);
  itkNewMacro(MyObject);

protected:
  MyObject()
  {
    this->AddObserver(itk::ModifiedEvent(), [](const auto &) { std::cout << "Modified!\n"; });
  }

  ~MyObject() override = default;
};


TEST(MyObject, Modified)
{
  auto myObject = MyObject::New();
}

When using the current main revision (and probably any released ITK version), MyObject::New() will call InvokeEvent(ModifiedEvent()) during the default-construction of its base class, itk::Object. However, that is still before the this->AddObserver call. During default-construction of itk::Object, it does not yet have any observers.

So MyObject::New() won't trigger a call to std::cout << "Modified!\n" anyway.