Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replaced uint with size_t #3640

Merged
merged 1 commit into from
May 29, 2023
Merged

replaced uint with size_t #3640

merged 1 commit into from
May 29, 2023

Conversation

guj
Copy link
Contributor

@guj guj commented May 26, 2023

as reported in:
#3638

@eisenhauer
Copy link
Member

@guj I'd see this as a bug, so maybe target to release_29 rather than master(so that it goes into the 2.9.1 release) and tag it as backport to master (and @vicentebolea will manage that bit) ?

@guj
Copy link
Contributor Author

guj commented May 26, 2023

@guj I'd see this as a bug, so maybe target to release_29 rather than master(so that it goes into the 2.9.1 release) and tag it as backport to master (and @vicentebolea will manage that bit) ?

Fine with me. Double checking with @vicentebolea. Shall I merge to release_29 instead of master?

@vicentebolea
Copy link
Collaborator

vicentebolea commented May 26, 2023

master, this PR changes the signature of public headers functions.

@guj
Copy link
Contributor Author

guj commented May 26, 2023

master, this PR changes the signature of public headers functions.

OK Thanks!

@vicentebolea
Copy link
Collaborator

Do we need this for the next patch release?

@guj
Copy link
Contributor Author

guj commented May 26, 2023

Do we need this for the next patch release?

Guess not urgently.

@eisenhauer
Copy link
Member

Actually, I think I need to push back on the public headers argument. I don't see that this is in any way a "public" header. It doesn't get installed, so it's not going to be referenced outside the ADIOS build. The two functions affected are not consumed outside of the h5vol build. Is there a conceivable way that putting this into a release would do more than let ADIOS compile in an environment in which it is currently broken?

@vicentebolea
Copy link
Collaborator

. I don't see that this is in any way a "public" header.

@eisenhauer you are right, my bad, I thought that this header gets installed but it is not the case. I am not too familiar about the H5Vol component, but I can see now that it is intended to be used as a plugin.

@guj This is change is in an internal function that does not affect the ABI of the plugin thus it should be good to go :). Please merge to master and I will backported with other MRs to release_29.

@guj
Copy link
Contributor Author

guj commented May 29, 2023

@eisenhauer @vicentebolea
Please approve so I can merge :)

@ax3l
Copy link
Contributor

ax3l commented Jun 1, 2023

master, this PR changes the signature of public headers functions.

Just FYI, since this breaks compilation for 2.9.0 and is new therein, I will need to patch this into any (at least Win) package manager I find for 2.9.0 - so you can also break public API and add it to master @vicentebolea, so 2.9.1+ will have it :)

Please merge to master and I will backported with other MRs to release_29.

Oh, I should read to the end :)

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.

4 participants