Better error handling and better Comments #409

Guelakais · 2024-06-24T18:02:17Z

A lot of error handling in the code is solved via if-queries, although you could also do it via match cases match cases are more idiomatic. There are still parts of the code that I don't quite understand. I have tried to comment everything I understand as much as possible.

esteve

@Guelakais thanks for your PR. I don't like doing matches on booleans, it actually makes the code less readable. Could you just keep the comments? Thanks.

rosidl_runtime_rs/src/sequence.rs

Guelakais · 2024-06-24T19:17:33Z

@Guelakais thanks for your PR. I don't like doing matches on booleans, it actually makes the code less readable. Could you just keep the comments? Thanks.

I don't like that either. However, I am more of the opinion that an error-enum should be introduced anyway, which covers error handling better. The matches somehow serve as an aid to my thinking. A suggestion: If I haven't figured it out by Wednesday evening, I'll rebuild the entire PR clean and with comments only. Until then, we can also see whether the other PRs are running better.

esteve · 2024-06-24T19:28:08Z

@Guelakais I'm not going to approve a PR that has code similar to this:

    fn clone(&self) -> Self {
        let mut seq = Self::default();
        match T::sequence_copy(self, &mut seq) {
            true => seq,
            _ => panic!("Cloning Sequence failed"),
        }
    }

it just does not make sense, plus there's no way to change the definition of this function because it's part of the Clone trait, you can't change it and expect to continue working. All the functions you've changed are functions defined in their respective traits, so you can't return an error-enum if it's not part of the function signature in the trait.

Guelakais · 2024-06-24T19:33:46Z

@Guelakais I'm not going to approve a PR that has code similar to this:
    fn clone(&self) -> Self {
        let mut seq = Self::default();
        match T::sequence_copy(self, &mut seq) {
            true => seq,
            _ => panic!("Cloning Sequence failed"),
        }
    }
it just does not make sense, plus there's no way to change the definition of this function because it's part of the Clone trait, you can't change it and expect to continue working. All the functions you've changed are functions defined in their respective traits, so you can't return an error-enum if it's not part of the function signature in the trait.

Ok,

Guelakais · 2024-06-24T19:57:42Z

I have finally learnt how to remove git commits from my history. That makes a lot of things easier.

Guelakais

I looked at it and edited it. The changes are currently being imported.

rosidl_runtime_rs/src/sequence.rs

More Comments needed!

…ot explained at all.

To be honest, I still don't understand exactly what's happening here. All I know is that it's at least somewhat understandable what's going on here.

Guelakais

I've removed the corresponding commit

Client handle has no comments yet.

esteve · 2024-06-26T08:29:27Z

@Guelakais I'm moving this PR back to draft, let us know when you're done making changes and we'll review it.

Guelakais

could also be worse. It's ok.

Guelakais · 2024-06-26T18:30:30Z

Yes, no, it's fine. I'm done with that so far.

maspe36

Please follow the rust doc convention for linking to items by name
https://doc.rust-lang.org/rustdoc/write-documentation/linking-to-items-by-name.html

In general, I think our documentation should follow this high level guideline

Short summary of the intention of the function/struct/modules

On success, does this.

If there is a failure, here is what we do.

More tokio inspriation
https://docs.rs/tokio/latest/tokio/io/trait.AsyncBufRead.html#tymethod.poll_fill_buf

maspe36 · 2024-07-14T18:04:02Z

rclrs/src/client.rs

+    /// This implementation finalizes the `rcl_client_t` instance by calling `rcl_client_fini`.
+    /// It also acquires the entity lifecycle mutex to protect against the risk of global variables
+    /// in the rmw implementation being unsafely modified during cleanup.


This section is redundant with the safety section.

maspe36 · 2024-07-14T18:04:14Z

rclrs/src/client.rs

    // This uses pub(crate) visibility to avoid instantiating this struct outside
    // [`Node::create_client`], see the struct's documentation for the rationale


Just noticed this, this is actually just a plain comment — it won't be part of the generated docs.

maspe36 · 2024-07-14T18:07:20Z

rclrs/src/client.rs

+    /// If the `take_response` method returns a `ClientTakeFailed` error, it is treated as a
+    /// spurious wakeup and is not considered an error.


Instead of enumerating each possible result in the docs of many functions, consider just linking to RclrsError and in a future PR we can improve the docs for that.

Here is some inspiration from tokio
https://doc.rust-lang.org/nightly/std/io/enum.ErrorKind.html

maspe36 · 2024-07-14T18:17:58Z

rclrs/src/client.rs

+    /// Executes the client by taking a response from the underlying `rcl_client_t` instance
+    /// and handling it appropriately.
+    ///
+    /// This method attempts to take a response from the client using `take_response`. If a response
+    /// is successfully taken, it is processed by either calling the associated callback or sending
+    /// the response to the associated future.


These sections could be combined and condensed. The docs for take_response already detail quite a bit about whats going on under the hood with the rcl handle.

This should say something along the lines of...

Get a response from the rcl handle via take_response and execute the associate callback or future.

maspe36 · 2024-07-14T18:19:16Z

rclrs/src/clock.rs

    pub fn steady() -> Self {
        Self::make(ClockType::SteadyTime)
    }

    /// Creates a new Clock with `ClockType::RosTime` and a matching `ClockSource` that can be used
    /// to update it
+    ///
+    /// # Returns


There really is no need for the # Returns section. Rustdoc already does a great job showcasing the function signature which will include the return type (with a link to it!).

Sometimes the function signature + name is sufficient documentation and anything we add is just noise.

maspe36 · 2024-07-14T18:43:48Z

rosidl_runtime_rs/src/sequence.rs

@@ -489,11 +537,40 @@ impl Display for SequenceExceedsBoundsError {
        )
    }
 }
-
+/// Implements the `std::error::Error` trait for the `SequenceExceedsBoundsError` struct.


Please remove this comment, it duplicates the information in the line of code and doesn't provide any additional content.

maspe36 · 2024-07-14T18:46:50Z

rosidl_runtime_rs/src/sequence.rs

@@ -463,14 +511,14 @@ impl<T: SequenceAlloc> Iterator for SequenceIterator<T> {
        self.idx += 1;
        Some(elem)
    }
-
+    /// returns the remaining length of the sequence


This comment doesn't really explain much. Here is some inspiration from the iterator trait
https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.size_hint

Also, this is not something to be addressed in this PR but I wonder why Sequence doesn't actually implement iterator.

maspe36 · 2024-07-14T18:48:12Z

rosidl_runtime_rs/src/sequence.rs

 impl<T: SequenceAlloc> ExactSizeIterator for SequenceIterator<T> {
+    /// Returns the length of the sequence as unsigned integer.


If you want to document this function, then the comment should expand on more than just the function signature.

Checkout the rustdocs for std::Vec::len

maspe36 · 2024-07-14T18:50:34Z

rosidl_runtime_rs/src/sequence.rs

+/// Macro to provide default implementations for the `SequenceAlloc` trait for primitive types.
+///
+/// This macro generates an implementation of the `SequenceAlloc` trait for a given primitive type.
+/// It defines three extern "C" functions (`$init_func`, `$fini_func`, and `$copy_func`) that are
+/// linked from the "rosidl_runtime_c" library and used to initialize, finalize, and copy sequences
+/// of the specified type.
+///
+/// The `sequence_init` function allocates space for the sequence and sets its size and capacity.
+/// If the sequence's data pointer is null, it calls `$init_func` to allocate memory. Otherwise,
+/// it writes zero bytes to the existing memory region.
+///
+/// The `sequence_fini` function finalizes the sequence by calling `$fini_func`.
+///
+/// The `sequence_copy` function copies the contents of one sequence to another by calling
+/// `$copy_func`.
+///
+/// # Safety
+///
+/// The implementations of `sequence_init`, `sequence_fini`, and `sequence_copy` are marked as
+/// `unsafe` because they call the corresponding extern "C" functions, which may have undefined
+/// behavior if their preconditions are not met. However, the macro assumes that there are no
+/// special preconditions for these functions.
+///
+/// # Arguments
+///
+/// * `$rust_type:ty` - The Rust type for which the `SequenceAlloc` trait should be implemented.
+/// * `$init_func:ident` - The name of the extern "C" function used to initialize a sequence.
+/// * `$fini_func:ident` - The name of the extern "C" function used to finalize a sequence.
+/// * `$copy_func:ident` - The name of the extern "C" function used to copy a sequence.


Even with cargo doc --document-private-items (or --all), this won't be picked up as a docstring by rust doc. So consider just making it a regular comment.

maspe36 · 2024-07-14T18:53:01Z

rosidl_runtime_rs/src/sequence.rs

@@ -505,6 +582,21 @@ macro_rules! impl_sequence_alloc_for_primitive_type {
        }

        impl SequenceAlloc for $rust_type {
+            /// Initializes a sequence with a given size.


None of the docstrings in here will be picked up by rustdoc. I'd recommend trimming them down (the high level description and safety sections mainly) and turning them into regular comments.

esteve · 2024-08-12T11:29:35Z

@Guelakais are you still interested in getting this PR merged? Could you please address @maspe36 's feedback? Thanks.

esteve requested changes Jun 24, 2024

View reviewed changes

rosidl_runtime_rs/src/sequence.rs Outdated Show resolved Hide resolved

rosidl_runtime_rs/src/sequence.rs Outdated Show resolved Hide resolved

Guelakais force-pushed the better_error_handling branch from 0155b07 to 21f801e Compare June 24, 2024 19:56

Guelakais force-pushed the better_error_handling branch from 8289ccd to 56b0001 Compare June 24, 2024 20:40

Guelakais commented Jun 24, 2024

View reviewed changes

esteve requested changes Jun 25, 2024

View reviewed changes

rosidl_runtime_rs/src/sequence.rs Outdated Show resolved Hide resolved

GueLaKais added 11 commits June 25, 2024 14:01

Added Commentary to the size_hint method

18166cc

More Comments needed!

Added Commentary to fn len method

d4247d9

Added Commentary to a trait implementation

b38acdb

This macro is essential for the entire crate. Unfortunately, it was n…

f5d1fad

…ot explained at all.

Added commentary to ros dependency usages

48a511a

Added comments to Function

0ee9a7f

To be honest, I still don't understand exactly what's happening here. All I know is that it's at least somewhat understandable what's going on here.

Added comments to Function

4fbae08

To be honest, I still don't understand exactly what's happening here. All I know is that it's at least somewhat understandable what's going on here.

Added comments to Function

a0a8717

To be honest, I still don't understand exactly what's happening here. All I know is that it's at least somewhat understandable what's going on here.

rust doesn't provide rustdoc comments for extern block

1baa9d2

Added comments to Function

6972841

To be honest, I still don't understand exactly what's happening here. All I know is that it's at least somewhat understandable what's going on here.

Added comments to Function

1e0787b

To be honest, I still don't understand exactly what's happening here. All I know is that it's at least somewhat understandable what's going on here.

Guelakais force-pushed the better_error_handling branch from 56b0001 to 1e0787b Compare June 25, 2024 12:13

GueLaKais added 3 commits June 25, 2024 14:16

Added comments to Function

c345ee3

To be honest, I still don't understand exactly what's happening here. All I know is that it's at least somewhat understandable what's going on here.

fixes in commentary dependency call

d0c96eb

Trying to

ef8e9f8

Guelakais commented Jun 25, 2024

View reviewed changes

GueLaKais added 4 commits June 25, 2024 22:31

Added comments to clientHandle

5f41764

Client handle has no comments yet.

Added comments to clientHandle

262770f

Client handle has no comments yet.

Added more comments

9f2ec44

Added comments to the fn handle func in impl<T> Clientbase for Client<T>

be26da9

GueLaKais added 2 commits June 25, 2024 22:50

fixes

3eded95

To understand clientbase execute method better

9a03faf

Guelakais force-pushed the better_error_handling branch from 67d1e2a to 9a03faf Compare June 25, 2024 21:23

Added comments

140aa55

esteve marked this pull request as draft June 26, 2024 08:28

GueLaKais added 2 commits June 26, 2024 14:34

added comments to clock function

0ea5289

added comments to subscriptionhandle

0c2bb70

Guelakais requested a review from esteve June 26, 2024 13:18

Guelakais commented Jun 26, 2024

View reviewed changes

Guelakais marked this pull request as ready for review June 26, 2024 18:30

maspe36 reviewed Jul 14, 2024

View reviewed changes

Merge branch 'ros2-rust:main' into better_error_handling

e7e5a28

Guelakais closed this Oct 1, 2024

Guelakais deleted the better_error_handling branch October 1, 2024 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error handling and better Comments #409

Better error handling and better Comments #409

Guelakais commented Jun 24, 2024

esteve left a comment •

edited

Loading

Guelakais commented Jun 24, 2024

esteve commented Jun 24, 2024 •

edited

Loading

Guelakais commented Jun 24, 2024

Guelakais commented Jun 24, 2024

Guelakais left a comment

Guelakais left a comment

esteve commented Jun 26, 2024

Guelakais left a comment

Guelakais commented Jun 26, 2024

maspe36 left a comment

maspe36 Jul 14, 2024

maspe36 Jul 14, 2024

maspe36 Jul 14, 2024

maspe36 Jul 14, 2024

maspe36 Jul 14, 2024

maspe36 Jul 14, 2024

maspe36 Jul 14, 2024

maspe36 Jul 14, 2024

maspe36 Jul 14, 2024

maspe36 Jul 14, 2024

esteve commented Aug 12, 2024

		// This uses pub(crate) visibility to avoid instantiating this struct outside
		// [`Node::create_client`], see the struct's documentation for the rationale

		/// If the `take_response` method returns a `ClientTakeFailed` error, it is treated as a
		/// spurious wakeup and is not considered an error.

		impl<T: SequenceAlloc> ExactSizeIterator for SequenceIterator<T> {
		/// Returns the length of the sequence as unsigned integer.

Better error handling and better Comments #409

Better error handling and better Comments #409

Conversation

Guelakais commented Jun 24, 2024

esteve left a comment • edited Loading

Choose a reason for hiding this comment

Guelakais commented Jun 24, 2024

esteve commented Jun 24, 2024 • edited Loading

Guelakais commented Jun 24, 2024

Guelakais commented Jun 24, 2024

Guelakais left a comment

Choose a reason for hiding this comment

Guelakais left a comment

Choose a reason for hiding this comment

esteve commented Jun 26, 2024

Guelakais left a comment

Choose a reason for hiding this comment

Guelakais commented Jun 26, 2024

maspe36 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esteve commented Aug 12, 2024

esteve left a comment •

edited

Loading

esteve commented Jun 24, 2024 •

edited

Loading