Skip to content

Conversation

@franzpoeschel
Copy link
Contributor

This is a workaround/fix for #1221

enum class SetAttributeMode : char
{
WhileReadingAttributes,
FromPublicAPICall
Copy link
Member

Choose a reason for hiding this comment

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

Is that public API call both read and write? Might need a doxygen string, even though its readable already :)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, Set... so write

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's write-only in the public API, but during parsing, we emulate read_write mode and use setAttribute also internally

key, value, internal::SetAttributeMode::FromPublicAPICall);
}

// TODO explicitly instantiate Attributable::setAttribute for all T in Datatype
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 done in src/backend/Attributable.cpp

Suggested change
// TODO explicitly instantiate Attributable::setAttribute for all T in Datatype
// note: we explicitly instantiate Attributable::setAttributeImpl for all T in Datatype in Attributable.cpp

Copy link
Member

Choose a reason for hiding this comment

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

added to 2nd commit.

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.

Thank you, LGTM :)

@ax3l ax3l enabled auto-merge (squash) March 14, 2022 01:54
@ax3l
Copy link
Member

ax3l commented Mar 14, 2022

Will fix merge conflict from the merge of #1226

@ax3l ax3l force-pushed the workaround-no-safeguard-while-reading branch from b0cfdcd to 1be5918 Compare March 14, 2022 02:37
@ax3l ax3l disabled auto-merge March 14, 2022 02:40
@ax3l ax3l enabled auto-merge (squash) March 14, 2022 02:41
@ax3l ax3l merged commit 509cded into openPMD:dev Mar 14, 2022
@ax3l
Copy link
Member

ax3l commented May 25, 2022

I have trouble backporting this to the release-0.14.5 branch due to changes in include/openPMD/backend/Attributable.hpp

@ax3l ax3l modified the milestones: 0.14.6, 0.14.5 Jun 6, 2022
@ax3l
Copy link
Member

ax3l commented Jun 6, 2022

Applied to 0.14.5 via ax3l#6 🎉

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