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

Add example for parallel sort #15

Merged
merged 6 commits into from
Nov 16, 2024
Merged

Conversation

Connor-GH
Copy link
Contributor

It is now clearer how to use this library. The example(s) are built by default.

Connor-GH added 2 commits October 10, 2024 19:33
It is now clearer how to use this library. The example(s) are built by default.
@hkaiser
Copy link
Member

hkaiser commented Oct 11, 2024

@Connor-GH thanks for looking into this. @SAtacker would you mind haveing a look?

@pingu-73
Copy link
Contributor

pingu-73 commented Oct 11, 2024

@SAtacker @hkaiser do we need to explicitly add examples 'coz it's pretty clear from tests how to use this library they simply work as examples.

@pingu-73
Copy link
Contributor

@Connor-GH are you building hpx-rs on linux or mac ?

@Connor-GH
Copy link
Contributor Author

I build it on linux.

@pingu-73
Copy link
Contributor

I build it on linux.

could you please add instructions to build it on linux in the Readme.
Thanks

@Connor-GH
Copy link
Contributor Author

Connor-GH commented Oct 11, 2024

@SAtacker @hkaiser do we need to explicitly add examples 'coz it's pretty clear from tests how to use this library they simply work as examples.

HPX has its tests, and it has its examples. Tests are meant to be entirely functional. People aren't really supposed to look at them until a regression pops up. On the other hand, HPX's examples are fairly small (can fit on one screen) and have some important comments. The examples are there for people who just need to quickly pick something up with the library and need to learn by doing, rather than searching for a specific flag or something (which is what the documentation is for).

This is probably best demonstrated visibly. Here is a simple example from HPX that I learned from to help develop this example (notice the comments):

It should also be noted that examples should be separated from the implementation, but I don't think this matters wrt the testcases. Many languages have built-in unit tests, and I think putting them in the implementation file is fine. D does this for example in its standard library, but their tests are a bit more trivial but express the power of the API. There is a way of writing tests that can work as examples, and D does this this well, but these tests just serve to "get the thing done" with low regard for the potential of it being used to educate.

Maybe I am bikeshedding here, but there is a genuine case for both examples and tests.

@pingu-73
Copy link
Contributor

@SAtacker @hkaiser do we need to explicitly add examples 'coz it's pretty clear from tests how to use this library they simply work as examples.

HPX has its tests, and it has its examples. Tests are meant to be entirely functional. People aren't really supposed to look at them until a regression pops up. On the other hand, HPX's examples are fairly small (can fit on one screen) and have some important comments. The examples are there for people who just need to quickly pick something up with the library and need to learn by doing, rather than searching for a specific flag or something (which is what the documentation is for).

This is probably best demonstrated visibly. Here is a simple example from HPX that I learned from to help develop this example (notice the comments):

Maybe I am bikeshedding here, but there is a genuine case for both examples and tests.

i agree that examples are essential, but i don't think it's ideal to write one-liner examples as they don't add much value beyond what's already covered in the tests. It would be more beneficial to create more complex and practical examples. Since most of the functions maintain a similar structure and usage style as HPX it's ideal to refer to hpx documentation for understanding about that function (till the time we aren't done with hpx-rs documentation), more elaborate examples would help demonstrate their effective use.

@SAtacker what are your thoughts on it?

hpx-sys/src/lib.rs Outdated Show resolved Hide resolved
@SAtacker
Copy link
Contributor

@SAtacker @hkaiser do we need to explicitly add examples 'coz it's pretty clear from tests how to use this library they simply work as examples.

HPX has its tests, and it has its examples. Tests are meant to be entirely functional. People aren't really supposed to look at them until a regression pops up. On the other hand, HPX's examples are fairly small (can fit on one screen) and have some important comments. The examples are there for people who just need to quickly pick something up with the library and need to learn by doing, rather than searching for a specific flag or something (which is what the documentation is for).
This is probably best demonstrated visibly. Here is a simple example from HPX that I learned from to help develop this example (notice the comments):

Maybe I am bikeshedding here, but there is a genuine case for both examples and tests.

i agree that examples are essential, but i don't think it's ideal to write one-liner examples as they don't add much value beyond what's already covered in the tests. It would be more beneficial to create more complex and practical examples. Since most of the functions maintain a similar structure and usage style as HPX it's ideal to refer to hpx documentation for understanding about that function (till the time we aren't done with hpx-rs documentation), more elaborate examples would help demonstrate their effective use.

@SAtacker what are your thoughts on it?

I agree that trivial, one-liner examples may not add much value at this stage, as they are largely covered by the existing tests.
It would be more beneficial to focus on more complex and practical examples that showcase real-world use cases, especially considering that most of the functions in hpx-rs follow similar patterns and usage as in HPX.
For now, referring to HPX documentation for detailed function usage (until the HPX-RS documentation is ready) makes sense.
That said, since there’s already an example in this PR, I have no objections to including it for now.

…init()

Having to do all of the Args -> Vec<String> -> *mut *mut c_char conversions in every program using hpx-rs seems tedious. This wrapper helps to remove some of these ugly layers by moving them to the library rather than the user code
@Connor-GH
Copy link
Contributor Author

I apologize, I have been busy lately. I finally have a commit ready again for review though. By the time this PR is done, there should be no code that directly calls the ffi. IOW: all code needs to go through the "front door" of hpx_sys::, rather than hpx_sys::ffi::.

Things I don't like about this code that I am asking guidance on:

  • I would really prefer not to pass this function pointer through by another static variable, but it was the only way I could find to keep the API of fn(Vec) to the user. It feels really bad, please work with me on fixing this.
  • A bit too much unsafe code in the string conversion process and whatnot. Is there a safe way to go from Vec to *mut *mut c_char? It seems like there would be.

…afe part of this assignment is itself, not the right-hand side.
hpx-sys/src/lib.rs Outdated Show resolved Hide resolved
@Connor-GH
Copy link
Contributor Author

@SAtacker
Copy link
Contributor

How would we feel about this implementation?

https://github.com/Actyx/Actyx/blob/1f4bd6699a9345bde630248294b10bbc902d7230/rust/actyx/node-ffi/src/lib.rs#L70-L143

I lost the context, could you please remind?

hpx-sys/src/lib.rs Outdated Show resolved Hide resolved
@pingu-73
Copy link
Contributor

@SAtacker LGTM

@SAtacker SAtacker merged commit 3d53d6d into STEllAR-GROUP:main Nov 16, 2024
1 check passed
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