cogciprocate / ocl

OpenCL for Rust
Other
721 stars 75 forks source link

Fix incomplete links in documentation #217

Open zimeg opened 1 year ago

zimeg commented 1 year ago

Summary

:wave: Hello! This PR updates incomplete links in the documentation to point to the expected page.

Updated links

Here are the anchors to where these updates begin for each page:

Notes and questions

I'm slightly unsure of where the links in this section should lead, but am willing to make any suggested updates!

Formatting of the ocl::core::Status link is also troublesome and doesn't seem to want to appear inline... Would it make sense to add another line to reference these docs?

https://github.com/cogciprocate/ocl/blob/d0f4b61a872cb7e7fb4c04091a0d34ba774c6ec6/ocl/src/standard/device.rs#L329-L335

c0gent commented 1 year ago

Looks great.

Formatting of the ocl::core::Status link is also troublesome and doesn't seem to want to appear inline... Would it make sense to add another line to reference these docs?

Yes, that sounds good or whatever you think would be clearest.

I'm slightly unsure of where the links in this section should lead, but am willing to make any suggested updates!

Sorry which links?

zimeg commented 1 year ago

Thanks! Before making the inline link change, I wanted to share an answer and some findings for the following question:

Sorry which links?

For the ::list method, the links to .status() and ocl::core::Status are both pointing to missing links. I believe these resources might have moved in a refactoring of ocl_core.

Digging into the code, I think possible errors for ::list are of type ocl_core::functions::ApiError which implements .status()[^1], but this method might not have link-able documentation. If this seems correct, I can add and link to this documentation?

[^1]: ::list calls core::get_device_ids which sometimes returns an error from eval_errcode, which can be an ocl_core::functions::ApiError.

And at a glance, it seems that the ocl::core::Status is now represented as ocl_core::Status. Replacing this link and related text (ocl::core::Error::Status) should be good, but I'm not fully sure...

Based on the implementation of .status(), I'm also wondering if the following change should be made, removing the Option around Status[^2]:

  Calling `.status()` on the returned error will return an 
- `Option(ocl::core::Status)` which can be unwrapped then
+ `ocl_core::Status` which can be
  matched to determine the precise reason for failure.

[^2]: I believe the Option<Status> is instead returned by .api_status() of ocl_core::error::Error

Not meaning to turn these link updates into a code exploration, I'm just somewhat unfamiliar with the multi-crate structure :sweat_smile:

c0gent commented 1 year ago

Ok great. It's been a while, I'll have to refamiliarize myself when I have time. I'll get back to you as soon as I can.

Appreciate your help!