Skip to content

Default constructors for Series and SeriesIterator#955

Merged
ax3l merged 2 commits intoopenPMD:devfrom
franzpoeschel:default-constructors
Apr 7, 2021
Merged

Default constructors for Series and SeriesIterator#955
ax3l merged 2 commits intoopenPMD:devfrom
franzpoeschel:default-constructors

Conversation

@franzpoeschel
Copy link
Contributor

I've noticed that those can be useful in user code. E.g. in GAPD, openPMD is only one of the many reading routines, making it impossible to use our for(auto iteration: series.readIterations())… API, instead resorting to manually using the iterators. That is easier if the iterators are default-constructible.

@franzpoeschel franzpoeschel force-pushed the default-constructors branch from 5e0ba01 to 99e7467 Compare April 1, 2021 17:08
@ax3l ax3l self-requested a review April 2, 2021 09:00
* @return true If a Series has been opened for reading and/or writing.
* @return false If the object has been default-constructed.
*/
operator bool() const;
Copy link
Member

@ax3l ax3l Apr 2, 2021

Choose a reason for hiding this comment

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

Uff, but don't we have to check this one now at the beginning of every public Series method call?
Otherwise we can have an invalid Series object, don't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could theoretically do this check within Series::get and it would then happen automatically at every public Series method call. But I don't know if we should.
Basically, #886 applied shared_ptr semantics to our Series class and this PR just follows this one step further: You can default-construct a Series / a shared_ptr, but then it will be empty. You can construct a Series / a shared_ptr with content and only then can you use it. So, maybe we can add this to the documentation of Series that it strives to work like a shared_ptr on some internally-held resources?

Btw, reason for this change: In GAPD, there is a struct that holds all of the necessary data for doing IO. Adding the Series and the Series iterators to that struct means it is not default-constructible any more which is rather annoying to use.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the details.

My thinking is along the lines what we want to do with a default-constructed Series. Can we still open a proper data series with it later on? And what happens if someone uses it accidentally as if it was already holding a valid data series, would it give a clear error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we still open a proper data series with it later on?

Yep, by using operator=(),e.g.:

Series series;
if( use_hdf5 ) {
    series = Series( "data.h5", Access::CREATE );
}
else {
    series = Series( "data.bp", Access::CREATE );
}

And what happens if someone uses it accidentally as if it was already holding a valid data series, would it give a clear error message?

What would happen would currently depend on your STL implementation of shared_ptr, but it would be exactly what would happen upon dereference of an empty shared_ptr.

So both points mimic exactly the behavior of shared pointers.

Copy link
Member

@ax3l ax3l Apr 6, 2021

Choose a reason for hiding this comment

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

What would happen would currently depend on your STL implementation of shared_ptr, but it would be exactly what would happen upon dereference of an empty shared_ptr.

Hm, I fear that is undefined behavior and we should guard the user from it with an exception instead, what do you think? https://en.cppreference.com/w/cpp/memory/shared_ptr

Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline: let's implement the check in Series::get and throw a clean exception if the Series is still default-constructed / owning a nullptr

@ax3l ax3l self-assigned this Apr 2, 2021
@franzpoeschel franzpoeschel force-pushed the default-constructors branch from 99e7467 to 3e67617 Compare April 6, 2021 10:21
Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Awesome 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants