bkietz commented on code in PR #46303:
URL: https://github.com/apache/arrow/pull/46303#discussion_r2075958465
##########
cpp/src/arrow/compute/kernels/vector_rank.cc:
##########
@@ -381,9 +381,13 @@ class RankMetaFunction : public
RankMetaFunctionBase<RankMetaFunction> {
}
RankMetaFunction()
- : RankMetaFunctionBase("rank", Arity::Unary(), rank_doc,
&kDefaultOptions) {}
+ : RankMetaFunctionBase("rank", Arity::Unary(), rank_doc,
GetDefaultOptions()) {}
Review Comment:
Yes, sorry; that wasn't intended as a serious proposal for this issue- it
was just another instance of me being surprised about ODR. I think your
`StaticOptionsInit` is fine; I can't convince myself looking at the standard
but [here's some
precedent](https://android-review.googlesource.com/c/platform/system/unwinding/+/3010579/2/libunwindstack/include/unwindstack/SharedString.h).
Given that the issue was raised in
[LLVM](https://github.com/llvm/llvm-project/issues/72361) and nobody exclaimed
about ODR, my intuition is just incorrect here. (Those issues discuss the
section group in which the static is emitted, which shouldn't bother us since
we're not planning `constexpr` construction of FunctionOptions)
Also `StaticOptionsInit` is neat, and maybe we should extend it to be
```cpp
template <typename T>
const T &Default() {
static const T kDefault;
return kDefault;
};
```
Then it might replace multiple instances of the same pattern in accessors
which must return `const std::shared_ptr<T>&` but don't have one, as in
[FileSource::path](https://github.com/apache/arrow/blob/main/cpp/src/arrow/dataset/file_base.h#L103-L108)
--
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]
