Update package modules w.r.t. public uses of SysCTypes #14599
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR updates the set of package modules whose interfaces require C types as defined in SysCTypes to include a `public use SysCTypes` so that clients of the modules will not have to use SysCTypes in order to make calls into them. Ideally, it would be nice if any Chapel-facing code in these modules did not require C types and that any conversion to C took place under the hood if need be, but that was a bigger change than I was able to take on here. And arguably, since they're just package modules, it's not so big of a deal. But we might consider making this an expectation for a given module before it transition to the standard modules directory.
|
@daviditen, @mppf, @ben-albrecht / @npadmana: If you have time today, would you give a quick look to the modules you've worked within in this PR to make sure the change seems appropriate? (and if not today, maybe after this merges?) |
mppf
approved these changes
Dec 6, 2019
daviditen
approved these changes
Dec 6, 2019
ben-albrecht
approved these changes
Dec 6, 2019
|
Thinking about this PR more over lunch, I wanted to give @daviditen credit for creating an HDF5 module where (unless I missed something) all the Chapel-facing functions use Chapel types and the only C types are in the C_HDF5 module. |
bradcray
changed the title
bradcray
pushed a commit
to bradcray/chapel
that referenced
this pull request
Dec 6, 2019
This should've been part of chapel-lang#14599, but it escaped me there. Here, I'm making the `use` of SysCTypes public since the MPI package's interface includes C types. Better would be to rewrite the package to avoid the need for C types from the Chapel user and convert under the covers before calling the external C routines, but that was more than I was able to take on today.
bradcray
added a commit
that referenced
this pull request
Dec 7, 2019
Add public use SysCTypes to MPI module [trivial, not reviewed] This should've been part of #14599, but it escaped me there. Here, I'm making the `use` of SysCTypes public since the MPI package's interface includes C types. Better would be to rewrite the package to avoid the need for C types from the Chapel user and convert under the covers before calling the external C routines, but that was more than I was able to take on today.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates the set of package modules whose interfaces require
C types as defined in SysCTypes to include a
public use SysCTypesso that clients of the modules will not have to use SysCTypes in
order to make calls into them.
Ideally, it would be nice if any Chapel-facing code in these modules
did not require C types and that any conversion to C took place under
the hood if need be, but that was a bigger change than I was able to take
on here. And arguably, since they're just package modules, it's not so big
of a deal. But we might consider making this an expectation for a given
module before it transition to the standard modules directory. The
HDF5 module was a nice example of this in that the top-level HDF5
module only needed a
private use SysCTypes;while the C_HDF5sub-module needed a
public use SysCTypes;for its clients.These updates were missed in PR #14524 since these modules are not
tested for vanilla linux64 runs. As a separate effort, I may look into
making some .noexec tests that run on linux64 to make sure we get to
the codegen stage for each of these libraries to catch such issues
ealier.