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 more documentation for the FFI API #3424

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Nov 17, 2023

In curl/curl#11203 @bagder pointed out that it's hard to know if code using the Hyper FFI interface is correct, because it is under-documented. I've tried to document things better based on my reading of the Rust code, client.c, and my knowledge of the Rust async ecosystem. @seanmonstar please take a look and let me know if I've gotten anything wrong. Thanks!

The general themes are:

  • I gave each struct a detailed description, including how to get one.
  • I added a list of methods for each struct. I know users can find the methods by searching for the struct name, but having them all in one place makes it much easier to quickly understand the operations on a struct.
  • I made task-creation functions consistently say "Creates a task that X" instead of "does X", to make it clearer that nothing happens right away.
  • I tried to cross-reference functions, to make it easy to figure out "how to get one" and "what to do next."
  • I specifically documented the relationship between a hyper_clientconn and a hyper_io, and why that means that a hyper_clientconn goes with a TCP/TLS connection for its lifetime.

@jsha jsha force-pushed the doc-ffi branch 2 times, most recently from c30f41c to 25db9fd Compare November 17, 2023 20:54
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Thank you so much! This is phenomenal. I had done #3373 to allow adding a bunch more docs and examples to each thing, without making the include file bigger and bigger. Your work here makes it much nicer <3

@seanmonstar seanmonstar merged commit d48528d into hyperium:master Nov 20, 2023
20 checks passed
@bagder
Copy link
Contributor

bagder commented Nov 24, 2023

A general problem is the complete lack of documentation for the C API for C developers. Everything so far is documented for and the reader is assumed to understand/prefer rust code. I don't.

@jsha
Copy link
Contributor Author

jsha commented Nov 24, 2023 via email

@seanmonstar
Copy link
Member

What I understood is that the function signature in the docs is in Rust, not in C. For instance, one shows:

#[no_mangle]
pub extern "C" fn hyper_buf_bytes(buf: *const hyper_buf) -> *const u8

Instead of the what is in the C header file:

const uint8_t *hyper_buf_bytes(const struct hyper_buf *buf);

I wonder if being able to output C for extern "C" functions in rustdoc would be something the maintainers would consider.


I do also hope to add actual usage examples written in C to each function, similar to curl's docs.

@bagder
Copy link
Contributor

bagder commented Nov 24, 2023

I'm even more basic. Where do I find the docs for the C API?

@jsha
Copy link
Contributor Author

jsha commented Nov 24, 2023 via email

@jsha
Copy link
Contributor Author

jsha commented Nov 24, 2023 via email

@seanmonstar
Copy link
Member

They are rendered here: https://docs.rs/hyper/latest/hyper/ffi/

That entry point includes some info about how to build, and then there's a list of structs, constants, and functions. Each one includes some sentences, and is also where examples can be rendered once we add them. The changes merged in this PR won't be available until another version is released.

@seanmonstar
Copy link
Member

I saw you tweaked things so the header file only gets the short summary of each function. I was surprised by that. I consider the header file to be the default source of documentation in the absence of a full on man page.

Oh, OK, we can switch it back. I kind of assumed that adding a bunch of bytes that are all comments/examples would bloat the file, and so in my head it made sense to just have the one sentence, and if you want to read more, look at the rendered docs. If that's abnormal, my mistake.

@seanmonstar
Copy link
Member

But I think it's probably too much complexity and too niche.

I'm less sure of that. As more and more Rust is used to interoperate with C, I think we (in hyper and rustls) will find places that the Rust tools could do a better job.

@bagder
Copy link
Contributor

bagder commented Nov 24, 2023

@bagder subject to the issues mentioned above (Rust types vs C types) you

Oh right. I think all the rust in those docs made me think those are not the C docs! Like how all the functions have rust prototypes, not C prototypes.

@jsha
Copy link
Contributor Author

jsha commented Nov 24, 2023

A very reasonable assumption! They are in fact Rust docs but because the functions are extern "C" they can be translated using some basic heuristics. Structs are structs, *mut foo == foo *, *const foo == const foo *, uint8_t == unsigned char, etc. Not ideal but it's something for now at least.

I've generated a copy of the latest docs, including my changes from the PR, and hosted them here:

https://rustdoc.crud.net/jsha/hyper-doc/hyper/ffi/index.html

For instance:

https://rustdoc.crud.net/jsha/hyper-doc/hyper/ffi/fn.hyper_body_data.html
https://rustdoc.crud.net/jsha/hyper-doc/hyper/ffi/fn.hyper_io_new.html

@jsha
Copy link
Contributor Author

jsha commented Nov 24, 2023

Another note on reading the rustdoc as a C developer: the structs will have a bunch of extra stuff below "Trait Implementations" that you can completely ignore. For instance https://rustdoc.crud.net/jsha/hyper-doc/hyper/ffi/struct.hyper_io.html has impl Read for hyper_io. That's all coincidental leakage from the internal Rust implementation and doesn't affect the C API at all.

What's relevant for the C API is the comment at the top of each struct page, and also the comments at the top of each function page.

I kind of assumed that adding a bunch of bytes that are all comments/examples would bloat the file, and so in my head it made sense to just have the one sentence, and if you want to read more, look at the rendered docs. If that's abnormal, my mistake.

That assumption makes sense, but until we have a better way to render C docs using C types and C function signatures, the header file is the best we've got. And it has the advantage of being automatically generated and fully official (if the function signatures don't match we would get compiler errors).

0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 12, 2024
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 16, 2024
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.

3 participants