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.

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

Add Deref(T *), to ease dereferencing a pointer safely #4278

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Nov 3, 2023

Deref(ptr) throws an exception when its argument (ptr) is null.

A similar function template is being used internally in elastix for quite a while: https://github.com/SuperElastix/elastix/blob/f64965f09eb0903e7bf1da4a112d914c7ac2ce22/Common/elxDeref.h

@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Enhancement Improvement of existing methods or implementation type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Nov 3, 2023
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.

Adding a follow-up commit with uses of this new functionality are highly welcome!

{
if (ptr == nullptr)
{
throw DerefError(__FILE__,
Copy link
Member

Choose a reason for hiding this comment

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

Is this file reported in the exception message, or the line where it is invoked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes indeed, both of them (source file and line number) are reported in the exception message.

Copy link
Member

Choose a reason for hiding this comment

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

I meant, is file == ".../Modules/Core/Common/include/itkDeref.h", or file == ".../my/file/in/my/code/which/tried/to/deref/null.cpp"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The itk::DerefError exception message proposed by this PR will have something like "Modules/Core/Common/include/itkDeref.h" in there. Of course, it would have been nicer to have something like "my/file/in/my/code/which/tried/to/deref/null.cpp". I think that can be implemented most elegantly by C++20 std::source_location 😃. Until then, I think it's still nice for users to know the location where the DerefError is thrown, in "itkDeref.h", so that they can put a breakpoint at that location, when they are debugging the problem.

Copy link
Member

Choose a reason for hiding this comment

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

Did you consider using the "itkSpecializedMessageExceptionMacro?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @blowekamp, please check my force-push!

@dzenanz
Copy link
Member

dzenanz commented Nov 6, 2023

/azp run ITK.Windows

@github-actions github-actions bot added the area:IO Issues affecting the IO module label Nov 6, 2023
@N-Dekker N-Dekker force-pushed the Deref branch 2 times, most recently from 791f0bc to 263dc98 Compare November 8, 2023 20:42
@N-Dekker
Copy link
Contributor Author

N-Dekker commented Nov 8, 2023

Update: I just removed Deref support for smart pointers, because there is some risk that it might lead to bugs. Suppose a function CreateObject() would return a smart pointer to a newly created object. The following might then cause ref to refer to a destructed object (after the line of code is executed):

auto & ref = Deref( CreateObject() );

On the other hand, Deref can certainly be used safely on raw pointers, as a raw pointer does not control the lifetime of the object pointed to.

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Nov 9, 2023

@dave3d Can you please explain how to fix the Spell Checking failures?

Run comment_spell_check \
  comment_spell_check \
    --miss \
    --prefix "gdcm" --exclude ThirdParty \
    --dict additional_dictionary.txt \
    --dict .github/workflows/itk_dict.txt \
    --suffix ".h" \
    --suffix ".hxx" \
    --suffix ".py" \
    Modules
  shell: /usr/bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.9.18/x64
    PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.9.18/x64/lib/pkgconfig
    Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.9.18/x64
    Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.9.18/x64
    Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.9.18/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.9.18/x64/lib
    Modules/Core/Common/include/itkDeref.h, 28

    Modules/Core/Common/include/itkDeref.h, 43
DerefError:

2 misspellings found

At https://github.com/InsightSoftwareConsortium/ITK/actions/runs/6803791308/job/18499888354?pr=4278

@dave3d
Copy link
Member

dave3d commented Nov 9, 2023

@dave3d Can you please explain how to fix the Spell Checking failures?

Run comment_spell_check \
  comment_spell_check \
    --miss \
    --prefix "gdcm" --exclude ThirdParty \
    --dict additional_dictionary.txt \
    --dict .github/workflows/itk_dict.txt \
    --suffix ".h" \
    --suffix ".hxx" \
    --suffix ".py" \
    Modules
  shell: /usr/bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.9.18/x64
    PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.9.18/x64/lib/pkgconfig
    Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.9.18/x64
    Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.9.18/x64
    Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.9.18/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.9.18/x64/lib
    Modules/Core/Common/include/itkDeref.h, 28

    Modules/Core/Common/include/itkDeref.h, 43
DerefError:

2 misspellings found

At https://github.com/InsightSoftwareConsortium/ITK/actions/runs/6803791308/job/18499888354?pr=4278

DerefError is a new keyword that should be added to the spelling dictionary. Add it to the file .github/workflows/itk_dict.txt.

@dzenanz
Copy link
Member

dzenanz commented Nov 9, 2023

Or perhaps use something which is "kosher", such as DereferenceError? It is a symbol which is not expected to be typed a lot.

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Nov 9, 2023

Thanks for your feedback, @dzenanz, @dave3d Personally I find "Deref" clear enough. When I google on "deref", it's all about dereferencing: https://www.google.com/search?q=deref And as "DerefError" is the error that may occur specifically when calling "Deref", I would prefer to stick with "DerefError". Hope that's acceptable to you both 🙏

@dave3d would it be sufficient to add "Deref" to .github/workflows/itk_dict.txt in order to use "DerefError"? I guess it knows "Error" already, right?

@dave3d
Copy link
Member

dave3d commented Nov 9, 2023

Thanks for your feedback, @dzenanz, @dave3d Personally I find "Deref" clear enough. When I google on "deref", it's all about dereferencing: https://www.google.com/search?q=deref And as "DerefError" is the error that may occur specifically when calling "Deref", I would prefer to stick with "DerefError". Hope that's acceptable to you both 🙏

@dave3d would it be sufficient to add "Deref" to .github/workflows/itk_dict.txt in order to use "DerefError"? I guess it knows "Error" already, right?

Yes, you can just add 'deref'.

`Deref(ptr)` throws an exception when its argument (`ptr`) is null.
Replaced the raw pointers in the VTKPolyDataMeshIO GoogleTest by C++ references,
as returned by `itk::Deref`. Removed the `ASSERT_NE(ptr, nullptr)` calls for
those pointers.

GoogleTest unit tests take care of catching exceptions, including those from
`itk::Deref`.
@N-Dekker N-Dekker changed the title ENH: Add Deref(const TPointer &) to ease dereferencing pointers safely Add Deref(T *), to ease dereferencing a pointer safely Nov 9, 2023
@N-Dekker N-Dekker marked this pull request as ready for review November 9, 2023 17:00
ASSERT_NE(outputMesh, nullptr);
const auto outputPoints = outputMesh->GetPoints();
ASSERT_NE(outputPoints, nullptr);
const auto & outputMesh = itk::Deref(reader->GetOutput());
Copy link
Member

Choose a reason for hiding this comment

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

This is a good demonstration of how this is supposed to be used. I would like to see some more instances of use, better demonstrating a need for a new function and exception class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @dzenanz

I use elastix::Deref extensively within elastix, e.g. (source):

elastix::Deref(MersenneTwisterRandomVariateGenerator::GetInstance()).SetSeed(...);

and (source)

MultiThreaderBase & multiThreader = elastix::Deref(this->ProcessObject::GetMultiThreader());
multiThreader.SetSingleMethod(...);
multiThreader.SingleMethodExecute();

Once itk::Deref is available, I would like to replace those elastix::Deref calls with itk::Deref. I feel that this function isn't really elastix specific. It could be useful for other ITK users as well. Does that answer your question?

@dzenanz dzenanz merged commit 85e29d0 into InsightSoftwareConsortium:master Nov 10, 2023
6 checks passed
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Dec 5, 2023
It appears somewhat risky to allow a smart pointer as `Deref` argument, as the original "smart" lifetime management may then be lost, in some cases.

Adjusted `Deref` calls in `AdvancedTransformAdapter` accordingly.

Following InsightSoftwareConsortium/ITK#4278 commit InsightSoftwareConsortium/ITK@042d423 "Add `Deref(T *)`, to ease dereferencing a pointer safely"
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Dec 5, 2023
It appears somewhat risky to allow a smart pointer as `Deref` argument, as the original "smart" lifetime management may then be lost, in some cases.

Adjusted `Deref` calls in `AdvancedTransformAdapter` accordingly.

Following InsightSoftwareConsortium/ITK#4278 commit InsightSoftwareConsortium/ITK@042d423 "Add `Deref(T *)`, to ease dereferencing a pointer safely"
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jun 6, 2024
Following ITK pull request InsightSoftwareConsortium/ITK#4278 commit InsightSoftwareConsortium/ITK@042d423 "ENH: Add `Deref(T *)`, to ease dereferencing a pointer safely".
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jun 6, 2024
Following ITK pull request InsightSoftwareConsortium/ITK#4278 commit InsightSoftwareConsortium/ITK@042d423 "ENH: Add `Deref(T *)`, to ease dereferencing a pointer safely".
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jun 10, 2024
Following ITK pull request InsightSoftwareConsortium/ITK#4278 commit InsightSoftwareConsortium/ITK@042d423 "ENH: Add `Deref(T *)`, to ease dereferencing a pointer safely".
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jun 10, 2024
Following ITK pull request InsightSoftwareConsortium/ITK#4278 commit InsightSoftwareConsortium/ITK@042d423 "ENH: Add `Deref(T *)`, to ease dereferencing a pointer safely".
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jun 10, 2024
Following ITK pull request InsightSoftwareConsortium/ITK#4278 commit InsightSoftwareConsortium/ITK@042d423 "ENH: Add `Deref(T *)`, to ease dereferencing a pointer safely".
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Jun 10, 2024
Following ITK pull request InsightSoftwareConsortium/ITK#4278 commit InsightSoftwareConsortium/ITK@042d423 "ENH: Add `Deref(T *)`, to ease dereferencing a pointer safely".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module area:IO Issues affecting the IO module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants