ARROW-1531: [C++] Return ToBytes by value from Decimal128

[cpcloud]

No description provided.

[wesm]

I don't think this has any performance implementations, does it? The downside of this change is that you can no longer pass l-values to this function

[cpcloud]

You can still pass them: http://en.cppreference.com/w/cpp/utility/forward

• If a call to wrapper() passes an rvalue std::string, then T is deduced to std::string (not std::string&, const std::string&, or std::string&&), and std::forward ensures that an rvalue reference is passed to foo.
• If a call to wrapper() passes a const lvalue std::string, then T is deduced to const std::string&, and std::forward ensures that a const lvalue reference is passed to foo.
• If a call to wrapper() passes a non-const lvalue std::string, then T is deduced to std::string&, and std::forward ensures that a non-const lvalue reference is passed to foo.

[wesm]

I mean that this code no longer works:

FixedSizeBinaryBuilder builder(type);

std::array<T, SIZE> val = ...;

builder.Append(val);

You would have to do builder.Append(std::move(val));. Might be wrong, should try compiling it before I put my foot in my mouth =)

[cpcloud]

That should still work, because that's a non-const lvalue which would be deduced to

std::array<uint8_t, 16>&

[wesm]

I tried compiling:

TEST_F(TestFWBinaryArray, Builder2) {
  InitBuilder(8);

  std::array<uint8_t, 8> val;

  ASSERT_OK(builder_->Append<8>(val));
}

../src/arrow/array-test.cc:1207:39: error: no matching member function for call to 'Append'
  do { ::arrow::Status s = (builder_->Append<8>(val)); if (!s.ok()) { return ::testing::internal::AssertHelper(::testing::TestPartResult::kFatalFailure, "../src/arrow/array-test.cc", 1207, "Failed") = ::testing::Message() << s.ToString(); } } while (0);
                            ~~~~~~~~~~^~~~~~~~~
../src/arrow/builder.h:743:10: note: candidate function [with NBYTES = 8] not viable: no known conversion from 'std::array<uint8_t, 8>' (aka 'array<unsigned char, 8>') to 'std::array<uint8_t, 8UL> &&' (aka 'array<unsigned char, 8UL> &&') for 1st argument
  Status Append(std::array<uint8_t, NBYTES>&& value) {

(this works if I add std::move)

[wesm]

Can you change the title? I'm going to cut the 0.7.0 RC after these tests pass and I can merge this

[wesm]

It might be worth having a void ToBytes(uint8_t* out) const version, too, for writing into preallocated memory

[cpcloud]

done

[wesm]

Appveyor build in progress, will merge once that's looking good: https://ci.appveyor.com/project/cpcloud/arrow/build/1.0.286

[cpcloud]

Appveyor build is green

[cpcloud]

I rebased this PR since then, just FYI