Add name metadata convenient constructors #310
Conversation
Alex-PLACET
force-pushed
the
add_name_metadata_convenient_constructors
branch
2 times, most recently
from
181e3f3 to
bc6077d
Compare
Alex-PLACET
force-pushed
the
add_name_metadata_convenient_constructors
branch
4 times, most recently
from
7d7b645 to
81fddde
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #310 +/- ##
=======================================
Coverage ? 91.87%
=======================================
Files ? 77
Lines ? 5770
Branches ? 0
=======================================
Hits ? 5301
Misses ? 469
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
It is usually more flexible to take arguments by value rather than by rvalue reference, at least in the public APIs (this way the user can decide to move or to copy), except if you explicitly want to to force the user to move (which I don't think is the case here). Doing the same in private APIs would be consistent, although not strictly required.
| inline null_array::null_array( | ||
| size_t length, | ||
| std::optional<std::string_view>&& name, | ||
| std::optional<std::string_view>&& metadata |
There was a problem hiding this comment.
These arguments should be passed by value so that the user can decide to copy or move.
| inline arrow_proxy null_array::create_proxy( | ||
| size_t length, | ||
| std::optional<std::string_view>&& name, | ||
| std::optional<std::string_view>&& metadata |
There was a problem hiding this comment.
Same here, prefer arguments by value rather than by rvalue reference.
| ArrowSchema schema = make_arrow_schema( | ||
| "n"sv, | ||
| std::forward<std::optional<std::string_view>>(name), | ||
| std::forward<std::optional<std::string_view>>(metadata), |
There was a problem hiding this comment.
It's not a deducing context, so no need for std::forward here. Prefer moving the arguments instead (or passing them by value if you need them later in the function).
| R&& values, | ||
| VB&& validity_input, | ||
| std::optional<std::string_view>&& name, | ||
| std::optional<std::string_view>&& metadata |
| std::move(offset_buffer), | ||
| std::forward<VB>(validity_input), | ||
| std::forward<std::optional<std::string_view>>(name), | ||
| std::forward<std::optional<std::string_view>>(metadata) |
There was a problem hiding this comment.
Prefer std::move here.
| arrow_proxy variable_size_binary_array_impl<T, CR, OT>::create_proxy( | ||
| R&& range, | ||
| std::optional<std::string_view>&& name, | ||
| std::optional<std::string_view>&& metadata |
There was a problem hiding this comment.
The 2 last arguments should be taken by value.
| values, | ||
| is_non_null, | ||
| std::forward<std::optional<std::string_view>>(name), | ||
| std::forward<std::optional<std::string_view>>(metadata) |
There was a problem hiding this comment.
Prefer std::move here.
| R&& range, | ||
| VB&& bitmap_input = validity_bitmap{}, | ||
| std::optional<std::string_view>&& name = std::nullopt, | ||
| std::optional<std::string_view>&& metadata = std::nullopt |
There was a problem hiding this comment.
The 2 last arguments should be taken by value
Alex-PLACET
force-pushed
the
add_name_metadata_convenient_constructors
branch
from
747bddf to
fb4075b
Compare
Sorry there is a lot of formatting, please ignore the whitespaces in the viewer's settings