ARROW-238: Change InternalMemoryPool::Free() to return Status::Invalid when ther… #102
Conversation
|
|
||
| virtual int64_t bytes_allocated() const = 0; | ||
| virtual uint64_t bytes_allocated() const = 0; |
There was a problem hiding this comment.
Using unsigned integers has far more cons than pros. The Google C++ guide indicates that signed integers should almost always be used. See https://google.github.io/styleguide/cppguide.html#Integer_Types
There was a problem hiding this comment.
Thanks. I've reverted this change.
jihoonson
changed the title
|
Unit testing seems to fail due to memory leak. I'll fix it soon. |
|
Broken test is fixed. |
| ASSERT_OK(pool->Allocate(100, &data)); | ||
| EXPECT_EXIT(pool->Free(data, 120), ::testing::ExitedWithCode(1), | ||
| ".*Check failed: \\(bytes_allocated_\\) >= \\(size\\)"); | ||
| pool->Free(data, 100); |
There was a problem hiding this comment.
Can you disable this test for release builds? See the NDEBUG define in CMakeLists.txt
There was a problem hiding this comment.
Thank you for your comment. Actually, I'm not an expert of C++, and would like to ask a question which may sound silly.
Does your comment mean that this unit test needs to be disabled for release builds? or, DCHECK in Free() should be disabled?
Would you please give me some backgrounds around this briefly?
There was a problem hiding this comment.
https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/logging.h#L45
When compiled in release mode, the DCHECK statements are omitted. Thus, the unit test will not pass. You can try it yourself by passing -DCMAKE_BUILD_TYPE=release
There was a problem hiding this comment.
Thanks! I disabled the test for release builds.
|
+1, thank you |
|
Thank you for your review. |
Also fixes few warnings in release mode build Author: Deepak Majeti <deepak.majeti@hpe.com> Closes apache#102 from majetideepak/ReleaseBuildErrors and squashes the following commits: b54f126 [Deepak Majeti] added api to read additional column metadata
Also fixes few warnings in release mode build Author: Deepak Majeti <deepak.majeti@hpe.com> Closes apache#102 from majetideepak/ReleaseBuildErrors and squashes the following commits: b54f126 [Deepak Majeti] added api to read additional column metadata Change-Id: Iddf193def450c092090a95758f27c1dcad87185a
Also fixes few warnings in release mode build Author: Deepak Majeti <deepak.majeti@hpe.com> Closes apache#102 from majetideepak/ReleaseBuildErrors and squashes the following commits: b54f126 [Deepak Majeti] added api to read additional column metadata Change-Id: Iddf193def450c092090a95758f27c1dcad87185a
Also fixes few warnings in release mode build Author: Deepak Majeti <deepak.majeti@hpe.com> Closes apache#102 from majetideepak/ReleaseBuildErrors and squashes the following commits: b54f126 [Deepak Majeti] added api to read additional column metadata Change-Id: Iddf193def450c092090a95758f27c1dcad87185a
Also fixes few warnings in release mode build Author: Deepak Majeti <deepak.majeti@hpe.com> Closes apache#102 from majetideepak/ReleaseBuildErrors and squashes the following commits: b54f126 [Deepak Majeti] added api to read additional column metadata Change-Id: Iddf193def450c092090a95758f27c1dcad87185a
…r substr_index (apache#102) * Fix a tiny issue * Implement substr_index in a func. holder * Fix compile issue * Fix other compile issues * Fix unit test issue * Fix bugs found in tests * Add some checks in ut
…e is insufficient memory.