Follow-up of frontend redesign: Remove unnecessary pointer members#936
Follow-up of frontend redesign: Remove unnecessary pointer members#936ax3l merged 7 commits intoopenPMD:devfrom
Conversation
This makes the class a little bit smaller (352 -> 344 bytes on my machine)
include/openPMD/Series.hpp
Outdated
| mapped_type & | ||
| operator[]( key_type && key ) override; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Needed to move to front now since m_writeIterations is now a plain member of SeriesData. We could use a unique pointer to avoid this.
There was a problem hiding this comment.
I think a plain member is quite good.
We could move the WriteIterations class in its own .hpp/.cpp files, though.
ax3l
left a comment
ax3l
left a comment
There was a problem hiding this comment.
This is great!
I added two minor updates inline.
If you like to merge Writable and AttributableData I am ok in a follow-up. If you like we can also discuss it briefly on Tuesday first.
include/openPMD/Series.hpp
Outdated
| mapped_type & | ||
| operator[]( key_type && key ) override; | ||
| }; | ||
|
|
There was a problem hiding this comment.
I think a plain member is quite good.
We could move the WriteIterations class in its own .hpp/.cpp files, though.
franzpoeschel
force-pushed
the
topic-remove-shared-pointers
branch
from
36fdc71 to
2f123e0
Compare
franzpoeschel
force-pushed
the
topic-remove-shared-pointers
branch
from
2f123e0 to
199f65d
Compare
include/openPMD/ReadIterations.hpp
Outdated
| #include "openPMD/Iteration.hpp" | ||
| #include "openPMD/Series.hpp" | ||
|
|
||
| /** Writing side of the streaming API. |
There was a problem hiding this comment.
If you add a doxygen block like this here, it will "pop down" to the next best match, which is namespace openPMD.
Do you like to make this a block for the file itself? In that case do this:
| /** Writing side of the streaming API. | |
| /** @file | |
| * | |
| * Writing side of the streaming API. |
There was a problem hiding this comment.
Copy paste error, that's not supposed to be there. Thanks for noticing.
src/ReadIterations.cpp
Outdated
| { | ||
| return SeriesIterator::end(); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
| } | |
| } // namespace openPMD | |
src/WriteIterations.cpp
Outdated
| } | ||
| return res; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
EOF newlines are good for:
- compiler robustness (pre C++11, this was even required by the C++ standard)
- better
git diffs
I am adding a namespace comment here since that makes the }} more readable, since it has no indentation.
| } | |
| } // namespace openPMD | |
2 participants
This makes the members of
SeriesDataandAttributableDataplain members instead of shared pointers.TODO:
Also, I've noticed that we could theoretically merge
WritablewithAttributableData, simplifying our class structure somewhat.Writable m_writableis now a plain member ofAttributableData, it is neither movable nor copyable so it's not going anywhere and it has a pointer back to theAttributableData, so there's no real reason to keep them separate apart from the large diff that would be..