HuaHuaY commented on code in PR #51266:
URL: https://github.com/apache/arrow/pull/51266#discussion_r3969433391
##########
cpp/src/arrow/compute/kernels/codegen_internal.h:
##########
@@ -481,7 +481,7 @@ struct UnboxScalar<Decimal256Type> {
template <typename T, typename VisitFunc, typename NullFunc>
requires std::is_void_v<std::invoke_result_t<VisitFunc, typename
GetViewType<T>::T>>
-static void VisitArrayValuesInline(const ArraySpan& arr, VisitFunc&&
valid_func,
+void VisitArrayValuesInline(const ArraySpan& arr, VisitFunc&& valid_func,
Review Comment:
LLVM 23 added `-Wunused-template` to `-Wall`.
https://releases.llvm.org/23.1.0/tools/clang/docs/ReleaseNotes.html
> -Wunused-template is now part of -Wunused (which is enabled by -Wall).
It diagnoses unused function and variable templates with internal linkage,
which in a header is a latent ODR hazard. It can be disabled with
-Wno-unused-template.
([#202945](https://github.com/llvm/llvm-project/issues/202945))
I don't think there's any harm in removing `static`. Template functions are
inherently similar to `inline` functions, and ODR issues won't arise.
##########
cpp/src/gandiva/engine.cc:
##########
@@ -207,7 +209,12 @@ Status UseJITLinkIfEnabled(llvm::orc::LLJITBuilder&
jit_builder) {
static auto maybe_use_jit_link =
::arrow::internal::GetEnvVar("GANDIVA_USE_JIT_LINK");
if (maybe_use_jit_link.ok()) {
ARROW_ASSIGN_OR_RAISE(static auto memory_manager, CreateMemmoryManager());
-# if LLVM_VERSION_MAJOR >= 21
+# if LLVM_VERSION_MAJOR >= 23
Review Comment:
https://github.com/llvm/llvm-project/pull/192214 added one more parameter.
##########
cpp/src/gandiva/engine.cc:
##########
@@ -422,10 +432,15 @@ Status Engine::LoadPreCompiledIR() {
/// Parse the IR module.
llvm::Expected<std::unique_ptr<llvm::Module>> module_or_error =
llvm::getOwningLazyBitcodeModule(std::move(buffer), *context());
- // NOTE: llvm::handleAllErrors() fails linking with RTTI-disabled LLVM builds
- // (ARROW-5148)
- ARROW_RETURN_NOT_OK(VerifyAndLinkModule(*module_,
std::move(module_or_error)));
- return Status::OK();
+ ARROW_ASSIGN_OR_RAISE(
+ auto src_ir_module,
+ AsArrowResult(module_or_error, "Failed to verify and link module: "));
+
+ // Built-in bitcode is JIT-compiled on the runtime host. Do not retain the
target
+ // selected by Clang when the bitcode was built.
+ RemoveBuildTargetAttributes(*src_ir_module);
Review Comment:
The requirements for inlining have become stricter in LLVM 23.
https://releases.llvm.org/23.1.0/docs/ReleaseNotes.html#changes-to-the-llvm-ir
> alwaysinline no longer bypasses inlining compatibility checks based on
target features. Inlining will only be performed if it is safe to do so.
##########
cpp/src/arrow/compute/exec.cc:
##########
@@ -1185,16 +1185,6 @@ class ScalarAggExecutor : public
KernelExecutorImpl<ScalarAggregateKernel> {
const FunctionOptions* options_;
};
-template <typename ExecutorType,
Review Comment:
It's a template function which doesn't have any callers.
##########
cpp/src/gandiva/engine.cc:
##########
@@ -131,6 +131,8 @@ template <typename T>
arrow::Result<T> AsArrowResult(llvm::Expected<T>& expected,
const std::string& error_context) {
if (!expected) {
+ // NOTE: llvm::handleAllErrors() fails linking with RTTI-disabled LLVM
builds
Review Comment:
This comment was supposed to be moved here from line 425 during a previous
code refactoring, but it was overlooked.
##########
cpp/src/arrow/scalar.cc:
##########
@@ -1191,21 +1191,21 @@ constexpr int64_t kMillisecondsInDay = 86400000;
// date to date
template <typename To>
-enable_if_t<std::is_same<To, Date64Scalar>::value,
Result<std::shared_ptr<Scalar>>>
+enable_if_t<std::is_same<To, Date64Type>::value,
Result<std::shared_ptr<Scalar>>>
Review Comment:
This is another issue exposed by `-Wunused-template`. The template
instantiation type was incorrect here, causing the function that was supposed
to execute this path to fall through to the "NotImplemented" exception instead.
A unit test has been added.
--
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]
