ARROW-6190: [C++] Define and declare functions regardless of NDEBUG

[ozars]

No description provided.

[wesm]

Could you explain in the PR description how the status quo creates a problem?

[ozars]

Sure. That's the error I got:

test-arrowproject.cpp:(.text._ZN5arrow22FixedSizeBinaryBuilder12UnsafeAppendEN6nonstd7sv_lite17basic_string_viewIcSt11char_traitsIcEEE[_ZN5arrow22FixedSizeBinaryBuilder12UnsafeAppendEN6nonstd7sv_lite17basic_string_$
iewIcSt11char_traitsIcEEE]+0xd1): undefined reference to `arrow::FixedSizeBinaryBuilder::CheckValueSize(long)'

This happened after I used FixedSizeBinaryBuilder::Append<NSIZE> which calls FixedSizeBinaryBuilder::UnsafeAppend(util::string_view), which in terms tries to call FixedSizeBinaryBuilder::CheckValueSize(long) if NDEBUG isn't defined. However NDEBUG isn't defined during Release build.

arrow/cpp/src/arrow/array/builder_binary.h

Lines 257 to 262 in a0ee637

	void UnsafeAppend(util::string_view value) {
	#ifndef NDEBUG
	CheckValueSize(static_cast<size_t>(value.size()));
	#endif
	UnsafeAppend(reinterpret_cast<const uint8_t*>(value.data()));
	}

arrow/cpp/src/arrow/array/builder_binary.cc

Lines 223 to 227 in a0ee637

	#ifndef NDEBUG
	void FixedSizeBinaryBuilder::CheckValueSize(int64_t size) {
	DCHECK_EQ(size, byte_width_) << "Appending wrong size to FixedSizeBinaryBuilder";
	}
	#endif

I can try adding a minimal example in the afternoon if needed.

[wesm]

The issue is clear now. We'll want to add this information to the PR description for the sake of the changelog. I'll let @pitrou review

[codecov-io]

Codecov Report

Merging #5049 into master will increase coverage by 1.59%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5049      +/-   ##
==========================================
+ Coverage   87.59%   89.19%   +1.59%     
==========================================
  Files        1009      727     -282     
  Lines      143902   103009   -40893     
  Branches     1418        0    -1418     
==========================================
- Hits       126056    91882   -34174     
+ Misses      17484    11127    -6357     
+ Partials      362        0     -362

Impacted Files	Coverage Δ
cpp/src/arrow/array/builder_binary.cc	90% <ø> (ø)	⬆️
cpp/src/arrow/array/builder_binary.h	92.92% <ø> (ø)	⬆️
cpp/src/arrow/util/utf8.h	93.33% <ø> (ø)	⬆️
cpp/src/arrow/util/utf8.cc	100% <ø> (ø)	⬆️
cpp/src/arrow/json/converter.cc	90.05% <0%> (-1.76%)	⬇️
cpp/src/arrow/json/chunked-builder.cc	79.91% <0%> (-1.68%)	⬇️
go/arrow/ipc/writer.go
js/src/util/fn.ts
go/arrow/memory/memory_avx2_amd64.go
rust/datafusion/src/execution/filter.rs
... and 279 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b98a560...f4e371e. Read the comment docs.

[pitrou]

Thanks for noticing this. The PR looks good to me.

[ozars]

Thanks.