ARROW-1211: [C++] Enable builder classes to automatically use the default memory pool #923
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xhochy
requested changes
Aug 1, 2017
cpp/src/arrow/builder.h
Outdated
Member
There was a problem hiding this comment.
I would rather prefer a second internal macro:
#ifdef ARROW_NO_DEFAULT_MEMORY_POOL
#define ARROW_ARG_MEMORY_POOL MemoryPool* pool
#else
#define ARROW_ARG_MEMORY_POOL MemoryPool* pool = default_memory_pool()
#endif
This would avoid the if…else…endif duplications here.
Member
Author
There was a problem hiding this comment.
Fixing. I may need to twiddle the Doxyfile a little bit so it expands the macro properly
…ssing explicitly. Add ARROW_NO_DEFAULT_MEMORY_POOL define. Flip builder constructor order, deprecate old constructors. README documentation Change-Id: Ie640d044a2f078127e81072b81e94ee20ebb9544
…ro expansion to Doxyfile Change-Id: Ie632a77e3d8530a87bdab53f53ffa1f2b48c0ef9
Change-Id: I9dea1f4b5acc2d1b7f5b7cf6556031745d7fc858
Change-Id: I15c66cb3d32bbaa3a0b2b7a9fc9547fc9b81b487
Change-Id: I4847b207b46f5b70922bb31d5bf3120a3d42a265
kou
reviewed
Aug 2, 2017
ci/travis_before_script_c_glib.sh
Outdated
| pushd $ARROW_C_GLIB_DIR | ||
|
|
||
| export ARROW_C_GLIB_CFLAGS="-DARROW_NO_DEPRECATED_API" | ||
| export ARROW_C_GLIB_CXXFLAGS="-DARROW_NO_DEPRECATED_API" |
Member
There was a problem hiding this comment.
How about calling ./configure with CFLAGS and CXXFLAGS like the following?:
CONFIGURE_OPTIONS="$CONFIGURE_OPTIONS CFLAGS=-DARROW_NO_DEPRECATED_API"
CONFIGURE_OPTIONS="$CONFIGURE_OPTIONS CXXFLAGS=-DARROW_NO_DEPRECATED_API"
./configure $CONFIGURE_OPTIONSIf we use generic configure options, we don't need to introduce Arrow specific variable such as ARROW_C_GLIB_CFLAGS.
Change-Id: Iadb85d7df49f4fd79812fc6738ae891555cd8a68
Member
Author
|
Appveyor build: https://ci.appveyor.com/project/wesm/arrow/build/1.0.838. Will merge when completed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I added the
ARROW_NO_DEFAULT_MEMORY_POOLdefine option to disable this in third party use.I also flipped the order of arguments to the builder constructors to be a bit more natural. I don't feel strongly about this, but it does make the code a bit nicer:
versus