Skip to content

Make switchType more comfortable to use#931

Merged
ax3l merged 6 commits intoopenPMD:devfrom
franzpoeschel:fix-switchType
Feb 23, 2021
Merged

Make switchType more comfortable to use#931
ax3l merged 6 commits intoopenPMD:devfrom
franzpoeschel:fix-switchType

Conversation

@franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Feb 18, 2021

This PR:

  • moves the switchType function templates to a separate header DatasetHelpers.hpp
  • adds new switchType versions for ADIOS2, this allows to throw out lots of annoying redirection SFINAE helper structs
  • makes switchType less annoying to use:
    1. The return type needs not be given as a template parameter explicitly any more
    2. Throwing errors upon detecting an unknown datatype is now done automatically if the switchType action struct has an errorMsg member

TODO:

  • Apply changes in JSON backend
  • Cleanup

This is slightly API breaking since the template is no longer exposed to users. Users may still include the header explicitly if they need it.

@ax3l ax3l self-requested a review February 18, 2021 20:24
@ax3l ax3l self-assigned this Feb 18, 2021
@franzpoeschel franzpoeschel force-pushed the fix-switchType branch 2 times, most recently from 20246fb to 824b591 Compare February 19, 2021 13:07
Clang 5 produces linking errors from this, so we can't actually apply
it.
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.

Looks great, minor wish about the macro-usage inline & some cleanup suggestions :)

const Parameter< Operation::WRITE_DATASET > & parameters
);

std::string errorMsg = "JSON: writeDataset";
Copy link
Member

Choose a reason for hiding this comment

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

Cool, looks nice to use :)

You can const those members, I guess.

Suggested change
std::string errorMsg = "JSON: writeDataset";
std::string const errorMsg = "JSON: writeDataset";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, to be fair I'm not a huge fan of making non-static members const since that automatically deletes any operator= members. Since there are places where I store instances of these structs inside other structs (the ADIOS2 backend for example), I would prefer not to have some inconspicuous member silently delete those operators..

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, agreed.

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, looks great!

We should refactor some Python code where we are not yet using switchType, I think :)

@ax3l ax3l merged commit f6b0054 into openPMD:dev Feb 23, 2021
@ax3l
Copy link
Member

ax3l commented Feb 23, 2021

Ups, we get an ICC error in WarpX:

[ 12%] Building CXX object CMakeFiles/openPMD.dir/src/Format.cpp.o
/tmp/ci-mtwgsg2YYE/src/include/openPMD/DatatypeHelpers.hpp(86): error: static assertion failed with "[switchType] Action needs either an errorMsg member of type std::string or operator()<unsigned>() overloads."
          static_assert(
          ^
          detected during:
            instantiation of "ReturnType openPMD::detail::CallUndefinedDatatype<n, ReturnType, Action, Placeholder, Args...>::call(Action, Args &&...) [with n=1000, ReturnType=openPMD::Datatype, Action=openPMD::detail::BasicDatatype, Placeholder=void, Args=<>]" at line 259
            instantiation of "auto openPMD::switchType(openPMD::Datatype, Action, Args &&...)->decltype((<expression>)) [with Action=openPMD::detail::BasicDatatype, Args=<>]" at line 390 of "/tmp/ci-mtwgsg2YYE/src/src/Datatype.cpp"
/tmp/ci-mtwgsg2YYE/src/include/openPMD/DatatypeHelpers.hpp(91): error: class "openPMD::detail::BasicDatatype" has no member "errorMsg"
              "[" + std::string( action.errorMsg ) + "] Unknown Datatype." );
                                        ^
          detected during:
            instantiation of "ReturnType openPMD::detail::CallUndefinedDatatype<n, ReturnType, Action, Placeholder, Args...>::call(Action, Args &&...) [with n=1000, ReturnType=openPMD::Datatype, Action=openPMD::detail::BasicDatatype, Placeholder=void, Args=<>]" at line 259
            instantiation of "auto openPMD::switchType(openPMD::Datatype, Action, Args &&...)->decltype((<expression>)) [with Action=openPMD::detail::BasicDatatype, Args=<>]" at line 390 of "/tmp/ci-mtwgsg2YYE/src/src/Datatype.cpp"
/tmp/ci-mtwgsg2YYE/src/include/openPMD/DatatypeHelpers.hpp(86): error: static assertion failed with "[switchType] Action needs either an errorMsg member of type std::string or operator()<unsigned>() overloads."
          static_assert(
          ^
          detected during:
            instantiation of "ReturnType openPMD::detail::CallUndefinedDatatype<n, ReturnType, Action, Placeholder, Args...>::call(Action, Args &&...) [with n=0, ReturnType=openPMD::Datatype, Action=openPMD::detail::BasicDatatype, Placeholder=void, Args=<>]" at line 267
            instantiation of "auto openPMD::switchType(openPMD::Datatype, Action, Args &&...)->decltype((<expression>)) [with Action=openPMD::detail::BasicDatatype, Args=<>]" at line 390 of "/tmp/ci-mtwgsg2YYE/src/src/Datatype.cpp"
/tmp/ci-mtwgsg2YYE/src/include/openPMD/DatatypeHelpers.hpp(91): error: class "openPMD::detail::BasicDatatype" has no member "errorMsg"
              "[" + std::string( action.errorMsg ) + "] Unknown Datatype." );
                                        ^
          detected during:
            instantiation of "ReturnType openPMD::detail::CallUndefinedDatatype<n, ReturnType, Action, Placeholder, Args...>::call(Action, Args &&...) [with n=0, ReturnType=openPMD::Datatype, Action=openPMD::detail::BasicDatatype, Placeholder=void, Args=<>]" at line 267
            instantiation of "auto openPMD::switchType(openPMD::Datatype, Action, Args &&...)->decltype((<expression>)) [with Action=openPMD::detail::BasicDatatype, Args=<>]" at line 390 of "/tmp/ci-mtwgsg2YYE/src/src/Datatype.cpp"
compilation aborted for /tmp/ci-mtwgsg2YYE/src/src/Datatype.cpp (code 2)

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants