You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/08/12 07:16:17 UTC
[GitHub] [arrow] bdunlay opened a new pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
bdunlay opened a new pull request #7939:
URL: https://github.com/apache/arrow/pull/7939
This change replaces the NDEBUG flag with ARROW_DEBUG_BUILD in header
files, as well as in the pkg-config .pc file artifact produced during
builds. This eliminates leakage of the -DNDEBUG compiler flag into
downstream projects that use pkg-config to build their software against
arrow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bdunlay commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
bdunlay commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-672730870
Actually, given the approach here, it wouldn't need to change -- my change would sets 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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bdunlay commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
bdunlay commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-673047928
I will push a new change with that newline removed, but the linter is going to complain anyway because there are lots of changes that are not compliant with the linter in that cmake file that existed prior to my change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-673017463
Ah, yes, you are right, in those cases the `#ifdef` is pointless.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-672732537
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...).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-673049070
If you installed [archery](https://arrow.apache.org/docs/developers/archery.html), you can run `archery lint --cmake-format`. Otherwise we can do it for you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bdunlay edited a comment on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
bdunlay edited a comment on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-673047928
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bdunlay commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
bdunlay commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-673051995
I'll submit this for review as is if that's okay with you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bdunlay commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
bdunlay commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-672719666
> (also, you forgot to change the `.pc` file itself...)
Whoops, must have gotten lost when I revised my commit. Had it in the original.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-672718347
(also, you forgot to change the `.pc` file itself...)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-673015852
@bdunlay Where exactly? Do you have an example?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #7939:
URL: https://github.com/apache/arrow/pull/7939
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bdunlay commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
bdunlay commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-673015197
@pitrou Why are the asserts wrapped in NDEBUG? Isn't that redundant?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bdunlay commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
bdunlay commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-673067583
Hey look at that, it's happy now 😄
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou edited a comment on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
pitrou edited a comment on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-672714872
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bdunlay commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
bdunlay commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-673016929
For example the lines you pointed out to me: [src/arrow/util/trie.h](https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/trie.h#L106-L108)
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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bdunlay edited a comment on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
bdunlay edited a comment on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-672730870
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-673449181
@github-actions crossbow submit -g linux
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou edited a comment on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
pitrou edited a comment on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-672717556
It looks like the only place that would need fixing is in [arrow/util/trie.h](https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/trie.h#L106-L108).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bdunlay commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
bdunlay commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-673039693
Think the linter is mad at me for an extra newline in the cmake file. Will remove that w/ an update once CI completes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-672681194
https://issues.apache.org/jira/browse/ARROW-9698
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
kou commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-673105431
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-672714872
I disagree with these changes. There no is reason to rename NDEBUG to another symbol. We should simply ensure when the ABI is the same with and without NDEBUG.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bdunlay commented on a change in pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
bdunlay commented on a change in pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#discussion_r469065711
##########
File path: cpp/src/arrow/util/checked_cast.h
##########
@@ -32,28 +32,28 @@ inline OutputType checked_cast(InputType&& value) {
static_assert(std::is_class<typename std::remove_pointer<
typename std::remove_reference<OutputType>::type>::type>::value,
"checked_cast output type must be a class");
-#ifdef NDEBUG
Review comment:
CI failed here because it explicitly undefined NDEBUG on a release build. I don't think these are public headers, so I could omit the change here and leave it as it was.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #7939: ARROW-9698: [C++] Remove -DNDEBUG flag leak in .pc file
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7939:
URL: https://github.com/apache/arrow/pull/7939#issuecomment-672717556
It looks the only place that would need fixing is in [arrow/util/trie.h](https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/trie.h#L106-L108).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org