Fix destructor and add include_directories #44
Conversation
| @@ -18,6 +18,7 @@ find_library(ARROW_LIB arrow REQUIRED) | |||
| message(STATUS "Found arrow library at ${ARROW_LIB}") | |||
|
|
|||
| find_path(ARROW_INCLUDE_DIR arrow/type.h) | |||
| include_directories("${ARROW_INCLUDE_DIR}") | |||
There was a problem hiding this comment.
we are following the recommendations for modern/modular cmake, which says that all include_directories must be against a target only (within the project), and prefers taking dependency on targets (for external projects) instead of explicitly adding include-dirs/compile-flags/libs/..
What's the reason for this change ? Maybe, I could suggest alternatives.
There was a problem hiding this comment.
Interesting. I added this because cmake was unable to find arrow includes without it.
There was a problem hiding this comment.
is this a problem while building gandiva ? Or, a project/test that depends on gandiva ? If it's the latter, you could use the procedure we do for our integration tests.
target_link_libraries(${TEST_NAME} PRIVATE gandiva)
There was a problem hiding this comment.
This is a problem when building gandiva. I have arrow installed in a conda environment. I can provide additional steps to reproduce if needed.
There was a problem hiding this comment.
sorry for the delay, I was on vacation. Can you please provide the steps to reproduce ?
Also, can you please spin off a different PR for the memleak fix ?
There was a problem hiding this comment.
Yep. I should be able to get to this tonight.
| @@ -34,6 +34,8 @@ class Node { | |||
| explicit Node(DataTypePtr return_type) | |||
| : return_type_(return_type) { } | |||
|
|
|||
| virtual ~Node() = default; | |||
There was a problem hiding this comment.
thanks for fixing this !
|
These issues are resolved in apache/arrow#2558 |
This PR fixes a memory leak where destruction of pointers to
Nodes that areinstances of child classes are not called because
Node's destructor is notvirtual.