taepper commented on issue #51834:
URL: https://github.com/apache/arrow/issues/51834#issuecomment-6023732729
Uh, this turns out to be a lot trickier than I thought. The constraints that:
- `lhs` can be a declaration
- on error, we need to return without producing a result (might be non
default-constructible)
- the intermediate `Result<..>` should not outlive the statement
are extremely difficult to fulfil.
Also, you only mentioned the debug stack sizes, but this is also the case
for release builds. Tests of frame sizes in bytes for 8 `ARROW_ASSIGN_OR_RAISE`
of 128 bytes:
```
gcc -O0 1520
gcc -O2 1248
clang -O0 2360
clang -O2 1096
```
I started with a lambda version:
```
#define ARROW_ASSIGN_OR_RAISE(lhs, rexpr) \
::arrow::Status st; \
lhs = [&]() { \
auto&& _r = (rexpr); \
if (ARROW_PREDICT_FALSE(!_r.ok())) { \
st = _r.status(); \
return .. // what goes here? \
} \
return std::move(_r).ValueUnsafe(); \
}(); \
ARROW_RETURN_IF_(!st.ok(), st, ARROW_STRINGIFY(rexpr))
```
but that doesn't really work unless the `lhs`'s type is default
constructible, and even then could break current call sites
A bit of back and forth with an LLM showed me that this should be possible:
```
#define ARROW_ASSIGN_OR_RAISE(lhs, rexpr) \
lhs = __extension__({ \
auto&& _r = (rexpr); \
ARROW_RETURN_IF_(!_r.ok(), _r.status(), ARROW_STRINGIFY(rexpr)); \
std::move(_r).ValueUnsafe(); \
})
```
but only works for gcc and clang. We could then guard the implementation and
use this on these architectures.
Do you think this is acceptable? Other than that I can currently only
imagine introducing another macro that does not support declarations as the
`lhs`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
