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

Move C API to submodule #845

Merged
merged 8 commits into from
Jun 23, 2021
Merged

Move C API to submodule #845

merged 8 commits into from
Jun 23, 2021

Conversation

simonbyrne
Copy link
Collaborator

This moves all the C API functions and types to an API submodule (this was part of #834, but changed to not depend on #833).

The tests aren't yet passing as h5d_read_chunk in src/api/helpers.jl depends on get_chunk_length and get_chunk_index (defined in src/api_midlevel.jl). It doesn't really make sense to have the C API depend on higher-level API functions, so one of these should probably be moved: which would make more sense?

@simonbyrne
Copy link
Collaborator Author

h5d_read_chunk in src/api/helpers.jl depends on get_chunk_length and get_chunk_index (defined in src/api_midlevel.jl). It doesn't really make sense to have the C API depend on higher-level API functions, so one of these should probably be moved: which would make more sense?

Given that these functions perform allocations and use keyword arguments, I decided it made more sense to move them to the midlevel (and deprecated some function signatures which were a bit confusing and unnecessary).

@simonbyrne simonbyrne force-pushed the sb/api branch 3 times, most recently from 817f594 to ff3057d Compare June 23, 2021 19:02
@simonbyrne
Copy link
Collaborator Author

I think this is ready. @musm any thoughts?

src/deprecated.jl Outdated Show resolved Hide resolved
@simonbyrne
Copy link
Collaborator Author

simonbyrne commented Jun 23, 2021

One question is whether we should move src/error.jl into src/api/? That's the only place where the lower-level API accesses a higher one.

@musm
Copy link
Member

musm commented Jun 23, 2021

It's a good question. I was thinking the same, for the reason you mention. The question is whether we should use H5Error for all exceptions in the HDF5 and API module, if that's the case then we have to keep it as is. If we want to restrict the error to the inner API, , perhaps we should create an H5LibError <: H5AbstractError as a subtype?

@musm
Copy link
Member

musm commented Jun 23, 2021

Hmm, on second thought, I'm actually thinking it would be better to leave the custom exception handling in the API module, since it is strictly being used to handle internal hdf5 exceptions and wouldn't make sense to use outside.

@musm
Copy link
Member

musm commented Jun 23, 2021

Let me know what you think about the last commit.

Given that the error is isolated in its own API module., we could even reuse the same H5Error name in the upper level module HDF5 without issues, as a higher level Exception to deal with errors in that module, whereas HDF5.API.H5Error makes it clear it handles the internal API level errors.

@@ -35,9 +53,9 @@ const SHORT_ERROR = Ref(true)

function Base.showerror(io::IO, err::H5Error)
n_total = length(err)
print(io, "H5Error: ", err.msg)
print(io, "$(typeof(err)): ", err.msg)
Copy link
Member

@musm musm Jun 23, 2021

Choose a reason for hiding this comment

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

I used typeof here so that the full module path is printed:
image

I think this is a good idea in case we decide to use the same exception error name in the main module.

@musm
Copy link
Member

musm commented Jun 23, 2021

I think this PR is good to go, unless you think we should handle the internal errors in a different manner.

@simonbyrne
Copy link
Collaborator Author

LGTM

@musm musm merged commit 9e45452 into JuliaIO:master Jun 23, 2021
@mkitti mkitti mentioned this pull request Oct 22, 2021
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.

2 participants