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.

Sign up for GitHub

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

Jump to bottom

[MATLAB] Create a MATLAB_ASSIGN_OR_ERROR macro to mirror the C++ ARROW_ASSIGN_OR_RAISE macro #36249

Closed
kevingurney opened this issue Jun 22, 2023 · 1 comment · Fixed by #36273
Closed

[MATLAB] Create a MATLAB_ASSIGN_OR_ERROR macro to mirror the C++ ARROW_ASSIGN_OR_RAISE macro #36249

kevingurney opened this issue Jun 22, 2023 · 1 comment · Fixed by #36273
Assignees
@kevingurney
Labels
Component: MATLAB Type: enhancement
Milestone
13.0.0

Comments

@kevingurney
Copy link
Member

Describe the enhancement requested

Per the discussion in #36190 (comment), it would help with code conciseness and clarity if we created a macro like MATLAB_ASSIGN_OR_ERROR to mirror the design of the ARROW_ASSIGN_OR_RAISE macro.

The naming MATLAB_ASSIGN_OR_ERROR roughly matches the naming of the MATLAB_ERROR_IF_NOT_OK macro.

This will make working with arrow::Result types in the MATLAB interface code much easier.

Example:

MATLAB_ASSIGN_OR_ERROR(auto array, MakeArray(), error::FAILED_TO_MAKE_ARRAY);

Component(s)

MATLAB
@kevingurney kevingurney mentioned this issue Jun 22, 2023
Merged
@kevingurney
Copy link
Member Author

take

@github-actions github-actions bot mentioned this issue Jun 23, 2023
Merged
kou added a commit that referenced this issue Jun 27, 2023
@kevingurney @sgilmore10 @kou
GH-36249: [MATLAB] Create a MATLAB_ASSIGN_OR_ERROR macro to mirror …
d5d98ae 
…the C++ `ARROW_ASSIGN_OR_RAISE` macro (#36273)

### Rationale for this change

1. To make working with `arrow::Result<T>` values easier in the C++ code for the MATLAB interface, it would be useful to have a `MATLAB_ASSIGN_OR_ERROR` macro that mirrors the design of the [`ARROW_ASSIGN_OR_RAISE`](https://github.com/apache/arrow/blob/320ecbd119f26cb2f8d604ed84aae2559dbc0e26/cpp/src/arrow/result.h#L475) macro.
2. @ kou helpfully suggested this here: #36190 (comment).
3. There is already a [`MATLAB_ERROR_IF_NOT_OK`](https://github.com/apache/arrow/blob/e3eb5898e75a0b901724f771a7e2de069993a33c/matlab/src/cpp/arrow/matlab/error/error.h#L26) macro, so this would roughly follow the approach of that macro.

### What changes are included in this PR?

1. Added new macros `MATLAB_ASSIGN_OR_ERROR(lhs, rexpr, id)`, `MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT(lhs, rexpr, context, id)`, and `MATLAB_ERROR_IF_NOT_OK_WITH_CONTEXT(expr, context, id)`.
2. Added additional comments describing the error macros.
3. Updated call sites (i.e. `numeric_array.h`, `boolean_array.cc`, and `record_batch.cc`) where `arrow::Result<T>` is being returned to use new `MATLAB_ASSIGN_OR_ERROR` / `MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT` macros where possible.

**Example**

*`MATLAB_ASSIGN_OR_ERROR`*

```matlab
// Erroring from a static Proxy "make" member function 
libmexclass::proxy::MakeResult CustomProxy::make(const libmexclass::proxy::FunctionArguments& constructor_arguments) {
    ...
    MATLAB_ASSIGN_OR_ERROR(auto array, array_builder.Finish(), "arrow:matlab:array:builder:FailedToBuildArray");
    ...
}
``` 

*`MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT`*

```matlab
// Erroring from a non-static Proxy member function 
void CustomProxy::example(libmexclass::proxy::method::Context& context) {
    ...
    // context is passed in as an input argument to the non-static Proxy member function
    MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT(auto array, array_builder.Finish(), context, "arrow:matlab:array:builder:FailedToBuildArray");
    ...
}
``` 

### Are these changes tested?

Yes.

1. The client code that was changed to use `MATLAB_ASSIGN_OR_ERROR` / `MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT` compiles and works as expected.
2. **Note**: The `MATLAB_ERROR_IF_NOT_OK_WITH_CONTEXT` and `MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT` macros are intended to be used with *non-static* `Proxy` member functions. For reference, non-static `Proxy` member functions return an error ID to MATLAB by assigning to the `context.error` property of the input `libmexclass::proxy::method::Context` object (see [this example](https://github.com/mathworks/libmexclass/blob/77f3d72c22a9ddab7b54ba325d757c3e82e57987/example/proxy/Car.cpp#L46)). This is different than how errors are returned  to MATLAB from within a `Proxy` static `make()` method.

### Are there any user-facing changes?

Yes.

1. There are now new `MATLAB_ASSIGN_OR_ERROR` / `MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT` macros that can be used to simplify the process of extracting a value of type `T` from an expression that returns an `arrow::Result<T>` or returning an appropriate error to MATLAB if the status of the `Result` is not "OK".

### Future Directions

1. Consider modifying `mathworks/libmexclass` so that non-static `Proxy` member functions return a `Result`-like object, instead of requiring clients to assign to `context.error`. This might help avoid the need for two different "kinds" of macros - i.e. `MATLAB_ASSIGN_OR_ERROR` / `MATLAB_ASSIGN_OR_ERROR_WITH_CONTEXT` and `MATLAB_ERROR_IF_NOT_OK` / `MATLAB_ERROR_IF_NOT_OK_WITH_CONTEXT`.

### Notes

1. Thank you @ sgilmore10 for your help with this pull request!
* Closes: #36249

Lead-authored-by: Kevin Gurney <kgurney@mathworks.com>
Co-authored-by: Kevin Gurney <kevin.p.gurney@gmail.com>
Co-authored-by: Sarah Gilmore <sgilmore@mathworks.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Co-authored-by: Sarah Gilomore <sgilmore@mathworks.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@kou kou added this to the 13.0.0 milestone Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kevingurney kevingurney

Labels
Component: MATLAB Type: enhancement
Projects
None yet
Milestone
13.0.0
2 participants
@kou @kevingurney