js8544 commented on code in PR #39098:
URL: https://github.com/apache/arrow/pull/39098#discussion_r1434682136
##########
cpp/src/gandiva/engine.h:
##########
@@ -30,23 +35,34 @@
#include "gandiva/llvm_types.h"
#include "gandiva/visibility.h"
+namespace llvm::orc {
+class LLJIT;
+} // namespace llvm::orc
+
namespace gandiva {
/// \brief LLVM Execution engine wrapper.
class GANDIVA_EXPORT Engine {
public:
+ ~Engine();
llvm::LLVMContext* context() { return context_.get(); }
llvm::IRBuilder<>* ir_builder() { return ir_builder_.get(); }
LLVMTypes* types() { return &types_; }
- llvm::Module* module() { return module_; }
+
+ /// Retrieve LLVM module in the engine.
+ /// This should only be called before `FinalizeModule` is called
+ llvm::Module* module();
/// Factory method to create and initialize the engine object.
///
/// \param[in] config the engine configuration
/// \param[in] cached flag to mark if the module is already compiled and
cached
- /// \param[out] engine the created engine
- static Status Make(const std::shared_ptr<Configuration>& config, bool cached,
- std::unique_ptr<Engine>* engine);
+ /// \param[in] object_cache an optional object_cache used for building the
module
+ /// \return arrow::Result containing the created engine
+ static Result<std::unique_ptr<Engine>> Make(
Review Comment:
Is this a breaking change? We made a lot of Status->Result change before.
Typically we would keep both APIs, mark the Status one as deprecated, and
remove it in a later release. But it depends on whether Engine is only used
internally or not.
##########
cpp/src/gandiva/llvm_generator.h:
##########
@@ -47,15 +49,17 @@ class FunctionHolder;
class GANDIVA_EXPORT LLVMGenerator {
public:
/// \brief Factory method to initialize the generator.
- static Status Make(const std::shared_ptr<Configuration>& config, bool cached,
- std::unique_ptr<LLVMGenerator>* llvm_generator);
+ static Result<std::unique_ptr<LLVMGenerator>> Make(
Review Comment:
Similarly here.
##########
cpp/src/gandiva/engine.cc:
##########
@@ -31,6 +31,7 @@
#include <unordered_set>
#include <utility>
+#include <arrow/util/io_util.h>
Review Comment:
```suggestion
#include "arrow/util/io_util.h"
```
--
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]
