ARROW-85: memcmp can be avoided in Equal when comparing with the same …

[drankye]

Avoid memcmp when possible, if the two underlying buffers are the same.

[wesm]

Can you add a unit test for this case if there is not one already (hard to verify that the short circuit is hit, but good to have the test, anyway)? You will also need to make the code linting / style checks pass.

[emkornfield]

Going through the check mentioned here: https://github.com/apache/arrow/tree/master/cpp#continuous-integration will help avoid build failures. It looks like there is a formatting issue in your code

[emkornfield]

It looks like you indenting here is off, which could be one reason why check-format failed.

[drankye]

Thanks for the hint! Am checking with the CI instructions locally.

[drankye]

There is CI building failure with the error message like follows. Not sure about what the exact errors. Don't know how to run 'make format'. Would anyone help point it out? Thanks.

Built target lint
Scanning dependencies of target check-format
~/build/apache/arrow/cpp ~/build/apache/arrow/cpp-build
clang-format suggested changes, please run 'make format'!!!!
make[3]: *** [CMakeFiles/check-format] Error 1
make[2]: *** [CMakeFiles/check-format.dir/all] Error 2
make[1]: *** [CMakeFiles/check-format.dir/rule] Error 2
make: *** [check-format] Error 2
The command "$TRAVIS_BUILD_DIR/ci/travis_script_cpp.sh" exited with 2.

[drankye]

According to the mentioned Continuous Integration instructions, I know how to run the commands to check the styles. However, it's just troublesome to install clang stuffs in my environment(centos6.5). Hope to figure it out some time later.

[wesm]

This will be time well spent, as adding ASAN to the build setup will also help us with development.

One way of getting a working LLVM 3.x toolchain with minimal effort is using:

https://github.com/cloudera/native-toolchain

[drankye]

Thanks Wes for the pointer!! It does work for my setup.

[drankye]

Wonder why it still failed. Below is my running and looks like OK.
[root@zkdesk cpp]# make format
/home/workspace/arrow/cpp /home/workspace/arrow/cpp
/home/workspace/arrow/cpp
Built target format
[root@zkdesk cpp]# make check-format
Scanning dependencies of target check-format
/home/workspace/arrow/cpp /home/workspace/arrow/cpp
/home/workspace/arrow/cpp
Built target check-format

[emkornfield]

That is strange. Let me see if I can reproduce the failure locally. What version of clang-format did you install (the tests currently run with 3.7)?

[emkornfield]

After running "make format" did you commit the changes it made and push the new results here? When I ran make format it made the following changes: emkornfield@cf19f72
If you did push the commit with the updates after running "make format" the only thing I can think of is different versions for clang-format are yielding different results, which means we need to pick a common version to standardize on.

[drankye]

Thanks Micah for the help! I didn't realize 'make format' will also correct formats, not just checking. My clang-format is 3.8.0, after the running, the change does happen exactly as yours. I will push it as you suggested.

[drankye]

In addition to the desired correctness change, there're also quite a few minor adjustments on the include lines in some files. I'm not sure if they're invited by the new version of clang-format, but thought it would be good to avoid such. So as you said a common version may help with this.

[drankye]

Hmm, it failed again, without pointing the concrete error/warning out clearly. Am not familiar with travis-ci, but wonder if it can dig into the details for the exact place that causes the failure. This will be necessary to allow contributors to solve the issue by whatever means, basically correcting the formats.

[wesm]

I see a failing unit test:

6/15 Test  #7: buffer-test ......................***Failed    0.92 sec

[wesm]

NULL is not used in C++11. You don't need to set this variable here.

[drankye]

My 'make unittest' step running is passed. Also manually executed the buffer-test program on both Windows(built from MSVC) and CentOS(GCC/g++), both fine. Will update the codes per the recent comments though.

[root@zkdesk cpp]# ./build/debug/buffer-test
[==========] Running 4 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 4 tests from TestBuffer
[ RUN ] TestBuffer.Resize
[ OK ] TestBuffer.Resize (0 ms)
[ RUN ] TestBuffer.ResizeOOM
[ OK ] TestBuffer.ResizeOOM (0 ms)
[ RUN ] TestBuffer.EqualsWithSameContent
[ OK ] TestBuffer.EqualsWithSameContent (0 ms)
[ RUN ] TestBuffer.EqualsWithSameBuffer
[ OK ] TestBuffer.EqualsWithSameBuffer (0 ms)
[----------] 4 tests from TestBuffer (0 ms total)

[----------] Global test environment tear-down
[==========] 4 tests from 1 test case ran. (1 ms total)
[ PASSED ] 4 tests.

[wesm]

You have a memory leak. Run with valgrind

[drankye]

Yes I don't free the allocated buffers in the tests (probably intended). I will fix them but not sure that will fix the test failure.

[drankye]

Ah it finally passed for this simple change. Thanks Wes and Micah for the patience and helpful guides! Much learned...

[wesm]

Can you change these tests to use PoolBuffer instead of Buffer (and do so in the future)? That way memory will be automatically deallocated in destructors.

[drankye]

Sorry for the late and thanks Wes. I just attempted to change the codes using PoolBuffer, and found that the current codes are more readable regarding the test logic. I do agree for most tests that need buffer, PoolBuffer would be better to save the free. Sure I will use it in the future.

[wesm]

I left one minor comment. When you're done with that can you change the title of the patch to start with ARROW-85: (it's missing the colon), otherwise the patch merge tool will reject it.

[wesm]

No problem -- can you please change the title of the patch to start with ARROW-85:? Still missing a colon

[drankye]

Just checked the existing list of pulls and guess changing the title of this pull as you suggested may do the work. Done.

[wesm]

+1, thank you