ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file

[bdunlay]

This change eliminates the need to include the -DNDEBUG flag in arrow's .pc file, which leaks into downstream projects' builds unexpectedly. The CheckSize function definition is now included in both debug and release builds, though it will no-op in release builds (as expected). The flag is no longer required as a means to make the ABI consistent across build types for downstream projects, and so it is now removed.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-9698

[pitrou]

I disagree with these changes. There no is reason to rename NDEBUG to another symbol. We should simply ensure that the ABI is the same with and without NDEBUG.

[pitrou]

It looks like the only place that would need fixing is in arrow/util/trie.h.

[pitrou]

(also, you forgot to change the .pc file itself...)

[bdunlay]

(also, you forgot to change the .pc file itself...)

Whoops, must have gotten lost when I revised my commit. Had it in the original.

[bdunlay]

Actually, given the approach here, it wouldn't need to change -- my change would set a different flag rather than NDEBUG.

In any case, this change was one approach suggested in the jira issue, but I am happy to change it.

From the code you pointed out, the NDEBUG check could simply be removed (not sure why it is necessary, if NDEBUG would also disable the assert anyway?). Or, it could be moved inside of the function definition and break/do nothing.

Thoughts or preferences?

[pitrou]

I don't know why @kou suggested the flag change. If I look at headers in /usr/include on Ubuntu 18.04, several of them use NDEBUG in places: it doesn't seem to be causing any issues in practice.

As for arrow/util/trie.h, the method should simply always be defined even if not used (though I'm not even sure that matters, since it's an inline method...).

[bdunlay]

@pitrou Why are the asserts wrapped in NDEBUG? Isn't that redundant?

[pitrou]

@bdunlay Where exactly? Do you have an example?

[bdunlay]

For example the lines you pointed out to me: src/arrow/util/trie.h

If this is a debug build, the function is included in the header, and the assert is evaluated. If it's a release build, the function is omitted, and the assert is obviously never evaluated. But if you removed the macro completely, in a release build, the function exist but would no-op (assert disabled).

[pitrou]

Ah, yes, you are right, in those cases the #ifdef is pointless.

[bdunlay]

Think the linter is mad at me for an extra newline in the cmake file. Will remove that w/ an update once CI completes.

[bdunlay]

I will push a new change with that newline removed, but the linter is going to complain anyway because there are lots of linting issues that are not compliant with the linter in that cmake file that existed prior to my change.

[pitrou]

If you installed archery, you can run archery lint --cmake-format. Otherwise we can do it for you.

[bdunlay]

I'll submit this for review as is if that's okay with you.

[bdunlay]

Hey look at that, it's happy now 😄

[kou]

I suggested the renaming approach because using release build libarrow.so without NDEBUG increases needless (empty) CheckXXX() function calls.
If it's acceptable, just removing NDEBUG is OK to me.

[pitrou]

+1, thank you @bdunlay

[pitrou]

@github-actions crossbow submit -g linux