felipecrv commented on PR #41575:
URL: https://github.com/apache/arrow/pull/41575#issuecomment-2112704249
> Should the `VisitAllNestedListConfigurations` template function also be
moved to an anonymous namespace? It is directly called from the test function
(`vector_selection_test.cc`), so I have kept it as it currently is.
Sorry, I missed that it was called from others. We can make it a
non-template by using `std::function<...>` instead of inlining the lambda
expression. This is not performance-critical code, so it's not gonna be a
problem:
```diff
- /// \tparam Visit a function type with signature
- /// void(const std::shared_ptr<DataType>& inner_type,
- /// const std::vector<int>& list_sizes)
template <class Visit>
static void VisitAllNestedListConfigurations(
- const std::vector<std::shared_ptr<DataType>>& inner_value_types,
Visit&& visit,
+ const std::vector<std::shared_ptr<DataType>>& inner_value_types,
+ const std::function<void(const std::shared_ptr<DataType>& inner_type,
+ const std::vector<int>& list_sizes)> &visit,
int max_depth = 3, int max_power_of_2_size = 32) {
```
> Additionally, I made modifications to the `Makefile` to accommodate the
Windows CI process. Could you please confirm if this approach is correct?
We avoid relative paths in CMakeLists.txt because that gets very confusing
very quickly. You should list this new `.cc` file in
https://github.com/apache/arrow/blob/2ca9ad2861387a08244427eb1a2457c32a8ed31a/cpp/src/arrow/CMakeLists.txt#L637-L643
The trick to find where to list new `.cc` files is to grep for `.cc` files
in the same directory as yours. Things could be more complicated than that, but
it usually works.
--
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]
