Feature/#35 error wrapping

[JamesMc86]

Added the ability to wrap the function as in #35 but as a direct function on the error cluster.

@jkneer would love to have some feedback and see if you see any issue with this approach over a macro?

[jkneer]

Hi,

Just moving the parameters into the FnOnce is so much cleaner.
It seems also nice to implement it on the error. But it feels a bit off, like the error is wrapping my function? Hmm

Having the possibility to return something on error seems nice, but I fail to see the advantage. Also, I miss the comfort of having an automatic status reflecting the error status.

So

fn somecall() -> Result<()> {}

#[no_mangle]
pub extern "C" fn app_init(
  error: ErrorClusterPtr
) -> LVStatusCode {
with_lverrorhandling!(&error, 
|| {
// do whatever
somecall()?;
},())
}

This way I had the StatusCode for free. (For the status code I'm fine if it is either LVStatusCode::SUCCESS or something else. With the current implementation as a function, I think I cannot actually have the return value reflect the error code easily?

#[no_mangle]
pub extern "C" fn app_init(
  error: ErrorClusterPtr
) -> LVStatusCode {
   error.wrap_function( InternalError::Misc,
|| {
// do whatever
somecall()?;
}
)
}

If we model the error handling after the LV semantics, the wrap_function should always wrap the full function body. Is there then ever a reason to return a status different from "SUCCESS" or the error code (FAILURE) to the caller?

1. the DLL node has its own error line, for when the actual DLL call fails
2. Error on the line: nothing inside the wrapper should be executed. The function status should probably indicate that nothing has been executed. But any value different from SUCCESS should be fine. This will almost never happen, as most of the time the error line should also be wired to the error terminal of the DLL node.
3. Error happening inside the wrapper: we update the error to reflect that. If some error occured and you want to return a status,

I think I would prefer to get rid of the return_on_err parameter and return the status code of the generated error. I don't see when I would ever want to return something else. So if that ever happens I would prefer to do that as a manual implementation in that case instead of having to think about that return value every single time I use the wrapper.

[JamesMc86]

So sometimes I think I want to use the return position for actual data. For example just querying a numeric parameter or calling a constructor:

#[no_mangle]
extern "C" fn layout_new_single(
    mut error_cluster_ptr: ErrorClusterPtr,
    //parameters
) -> *mut LvLayout {
    error_cluster_ptr.wrap_function(std::ptr::null_mut(), || {
        //constructor
    })
}

I think not having this would be restrictive.

I like the idea of returning a status code automatically and perhaps there is an alternate version for that. But then, isn't that just a duplicate of the error cluster data anyway?

[jkneer]

I like the idea of returning a status code automatically and perhaps there is an alternate version for that. But then, isn't that just a duplicate of the error cluster data anyway?

A return status will probably mostly evaluated as a boolean expression. In most of my use cases just returning a boolean would be fine. Returning the status code achieves the same thing with the added value of carrying some information on the failure.

What if A is an Option, and for Option.is_none() we return the error code?

wrap_function(&mut self: LvErrorPtr, return_on_error: R, F) -> R
where
  F: FnOnce() -> Result<R, E>,  // this is important so we can use the try operator
  E: ToLvError
{
       if self.is_err() {
            return return_on_error;
        }
        match function() {
            Ok(value) => value,
            Err(error) => {
                let _ = error.write_error(self);
                if let Some(ret) = return_on_error {
                    return ret
                } else {
                    return self.code()
                }
            }
        }
}

Hmm thats a problem for the return type.

I'm wondering if it would be better to do it in two steps and use map_err to create the customized return on error.

[JamesMc86]

A return status will probably mostly evaluated as a boolean expression. In most of my use cases just returning a boolean would be fine. Returning the status code achieves the same thing with the added value of carrying some information on the failure.

So why would someone want to read this in the return position instead of reading the error cluster? This is what I'm missing.

I'm wondering if it would be better to do it in two steps and use map_err to create the customized return on error.

Yeah the consistent return type is the issue so it can't be the same function. You could have a core one that just returns the result then different actions on the back of that. I feel we could still wrap them in the API to reduce boilerplate (this has already made my project much easier to read) but sounds like we just need a couple of variants.

[JamesMc86]

I've added a wrap_return_status method which wraps the return function and returns the status instead. I think this is the most composable option.

You always have to be able to return something in an error in case which limited some other options. We could also have something that returns default.

But I think these cases cover it well, don't love the name but have nothing better right now!

I've been actively using it though and definately makes integration easier

[jkneer]

Looks good. Let's merge it in.