🔧 Improve error handling, return types, and add missing functions #12
Conversation
FlannyH
force-pushed
the
cleanup-new
branch
4 times, most recently
from
723ed8d to
886c394
Compare
src/lib.rs
Outdated
| bindings::IRBytecodeOwnership::None, | ||
| ); | ||
| bindings::IRBytecodeOwnership::Copy, |
There was a problem hiding this comment.
Maybe we should document why this is changing (with a comment what Copy means). Alternatively we could return an IRObject<'data> with _marker: PhantomData<&'data [u8]> :)
There was a problem hiding this comment.
About the PhantomData: if I understand this correctly, we can use that to keep track of the lifetime of the bytecode data slice, by adding an empty dummy field to the struct, which means we won't have to copy the bytecode?
| // The documentation is inconsistent about this function. | ||
| // The docs say `You must cast this pointer to the appropriate error payload struct for the error code`` | ||
| // but the example code just treats this as a char* like this: `printf("%s\n", (const char*)IRErrorGetPayload(pRootSigError));` | ||
| // Let's assume the example code is correct |
There was a problem hiding this comment.
We should request a clarification that explains for each IRErrorGetCode() how the payload should be interpreted. Maybe only linking gives an accompanying error message.
Besides, you're never using nor exposing this payload to the user, why?
src/lib.rs
Outdated
| // but the example code just treats this as a char* like this: `printf("%s\n", (const char*)IRErrorGetPayload(pRootSigError));` | ||
| // Let's assume the example code is correct | ||
| let payload = self.funcs.IRErrorGetPayload(self.me.as_ptr()) as *mut i8; | ||
| CString::from_raw(payload) |
There was a problem hiding this comment.
https://doc.rust-lang.org/std/ffi/struct.CString.html#method.from_raw
Retakes ownership of a CString that was transferred to C via CString::into_raw.
So that's a no :)
There was a problem hiding this comment.
I'm not sure what to do with this one
There was a problem hiding this comment.
CStr::from_ptr(). See last commit.
…-irobject Show how to store `IRCompiler` inside `IRObject` for reduced function complexity
|
I've manually applied the remainder of the review and am going to merge this. There's still quite a lot wrong with the API, but that's something I'll solve in future PRs to not conflate this too much. Most notably:
|
This pointer: - Wasn't created by `CString::into_raw()`; - Isn't owned by us, hence shouldn't be freed by us.
This PR improves error handling, using
thiserrorto return clearer error messages to the user, and adds some of the missing raytracing-related functions. Additionally, this PR movesmain.rstoExamples/compute-shader.rs