chore: return lvalue ref for cxx's scanbuilder functions #4306
Conversation
Signed-off-by: Xinyu Zeng <xinyuzeng@tencent.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Xinyu Zeng <xinyuzeng@tencent.com>
vortex-cxx/cpp/src/scan.cpp
Outdated
| ScanBuilder &&ScanBuilder::WithFilter(ExprType &&expr) && { | ||
| if constexpr (std::is_rvalue_reference_v<ExprType &&>) { | ||
| impl_->with_filter(std::forward<ExprType>(expr).IntoImpl()); | ||
| } else { | ||
| impl_->with_filter_ref(std::forward<ExprType>(expr).Impl()); | ||
| } |
There was a problem hiding this comment.
cxx does not support function overload (although I think technically it can do it), so we have to add separate ffi functions and do static dispatch here.
There was a problem hiding this comment.
I think the forwarding part is less important than the "reference collapsing" aspect such that the builder function can handle both rvalues and lvalues. Let's drop std::forward and a keep a single with_xyz not adding a ref version.
Just saw that you already added the ref versions. I think that looks good, even if a bit redundant.
There was a problem hiding this comment.
I think we should drop std::forward and simply leave the overload variants for both rvalue and lvalue ref of Expr. I am working on that now.
There was a problem hiding this comment.
If there are only two variants, and we still need specialization for both of them (to call the with_filter_ref ffi), then we do not need forward I think
There was a problem hiding this comment.
Would you mind taking a look at the modifications of my latest commit? I think it is cleaner
There was a problem hiding this comment.
If there are only two variants, and we still need specialization for both of them (to call the with_filter_ref ffi), then we do not need forward I think
Yes, this only works if the function we're forwarding to also supports r and l values. Thanks to C++ that's a silly amount of redudancy but this is fine for now. The most important part is that the API is hard to misuse now. We can iterate on making this more compact down the line.
There was a problem hiding this comment.
I'll have a second thought here also on whether C++23 might be ok to use so we could express this with reference collapsing and auto-deducing this in order to have a single func definition.
There was a problem hiding this comment.
I think C++ 20 may be ok but C++23 is too aggressive. As I know someone is still using C++17 (e.g., my current employer). Arrow-cpp recently had a vote to switch to 20 but it has not happened yet.
Signed-off-by: Xinyu Zeng <xinyuzeng@tencent.com>
Signed-off-by: Xinyu Zeng <xinyuzeng@tencent.com>
2 participants
rationales described here: #4050 (comment) #4050 (comment)