Skip to content

Conversation

AnidaKhizar
Copy link

@AnidaKhizar AnidaKhizar commented Oct 8, 2025

Ensuring the use of a compatible HDF5 API version during errors handling

List of things to check before making a PR

Before merging your code, please check the following:

  • you have added a line describing your changes to the Changelog;
  • you have added unit tests for any new or improved feature;
  • In case you updated dependencies, you have checked pdi/docs/CheckList.md
  • you have checked your code format:
    • you have checked that you respect all conventions specified in CONTRIBUTING.md;
    • you have checked that the indentation and formatting conforms to the .clang-format;
    • you have documented with doxygen any new or changed function / class;
  • you have correctly updated the copyright headers:
    • your institution is in the copyright header of every file you (substantially) modified;
    • you have checked that the end-year of the copyright there is the current one;
  • you have updated the AUTHORS file:
    • you have added yourself to the AUTHORS file;
    • if this is a new contribution, you have added it to the AUTHORS file;
  • you have added everything to the user documentation:
    • any new CMake configuration option;
    • any change in the yaml config;
    • any change to the public or plugin API;
    • any other new or changed user-facing feature;
    • any change to the dependencies;
  • you have correctly linked your MR to one or more issues:
    • your MR solves an identified issue;
    • your commit contain the Fix #issue keyword to autoclose the issue when merged.

Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution! Please use the script bin/test_indent to fix any indentation issue. I also have a few questions a propositions for modifications.

@AnidaKhizar AnidaKhizar changed the title Fix #567 Fix #567 Check of the HDF5 API version to use in error handler Oct 9, 2025
@AnidaKhizar
Copy link
Author

AnidaKhizar commented Oct 9, 2025

Thank you for your comments ! I can't apply the bin/test_indent as I'm on an old Fedora machine, with an old version of clang-format (version 12). Can the indentation be done with a different approach ?

@jbigot
Copy link
Member

jbigot commented Oct 9, 2025

I've applied the indentation

@AnidaKhizar
Copy link
Author

I've applied the indentation

OK thanks a lot !

I've moved the class at the end of the file as it uses Raii_hid, but I don't know if you'd rather have a forward declaration

/// The original handler data
void* m_old_data;

static unsigned get_api_version()
Copy link
Member

Choose a reason for hiding this comment

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

Could you just add a comment to document the purpose of the function please?

@jbigot
Copy link
Member

jbigot commented Oct 10, 2025

This looks pretty good to me in the code. We just need to handle the more "administrative" parts now (copyright, changelog, etc.)

@jbigot
Copy link
Member

jbigot commented Oct 14, 2025

@Yushan-Wang , I let you see about the next steps

Copy link
Member

@Yushan-Wang Yushan-Wang left a comment

Choose a reason for hiding this comment

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

Ready to go! Thanks!

@jmorice91
Copy link
Contributor

It will be nice to add a test also. Perhaps in a new PR?

@jbigot
Copy link
Member

jbigot commented Oct 17, 2025

It will be nice to add a test also. Perhaps in a new PR?

Indeed, I see the following checked:

  • you have added unit tests for any new or improved feature;

What does it refer to?

@Yushan-Wang
Copy link
Member

It will be nice to add a test also. Perhaps in a new PR?

Indeed, I see the following checked:

  • you have added unit tests for any new or improved feature;

What does it refer to?

I did not consider this a new feature, but rather a bug fix. So I didn't think of the test.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support both HDF5 API versions (1 and 2) in the hdf5 error handler

4 participants