-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-43209: [C++] Add lint for DCHECK in public headers #43248
Conversation
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format?
or
In the case of PARQUET issues on JIRA the title also supports:
See also: |
test | ||
arrow/util/hash.h | ||
arrow/python/util''')), | ||
(lambda x: re.match(_ASSIGN_OR_RAISE_REGEX, x), | ||
'Use ARROW_ASSIGN_OR_RAISE in header files', _paths('''\ | ||
arrow/result_internal.h | ||
test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File whose path containing test
and internal
will be excluded universally in lint_files
.
hash.h
is already gone.
|
A small cleanup. @pitrou would you like to take a look? Thanks. |
@github-actions crossbow submit -g cpp |
This comment was marked as outdated.
This comment was marked as outdated.
@github-actions crossbow submit -g cpp |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your good catch!
@github-actions crossbow submit -g cpp |
Revision: cb0e3b1 Submitted crossbow builds: ursacomputing/crossbow @ actions-84164aa0b0 |
@zanmato1984 I think we should avoid |
Thanks for reminding. That's very thoughtful and I have updated the PR description. Appreciate that! |
Thanks a lot @zanmato1984 ! |
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 12f68fc. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
I raised my question in #43209 about which I have always been curious. The top answer makes a good sense and after some searching in the code base, I found more evidence like:
arrow/cpp/src/arrow/public_api_test.cc
Lines 67 to 71 in 0372617
So I'm making the following changes to
DCHECK
macro family.What changes are included in this PR?
DCHECK
usage in public headers;DCHECK
usages in public (probably non-public, but at least not named withinternal
) headers;ifdef
protection forDCHECK
definition like we did forASSIGN_OR_RAISE
(arrow/cpp/src/arrow/result_internal.h
Lines 20 to 22 in 0372617
RETURN_NOT_OK
(arrow/cpp/src/arrow/status.h
Lines 80 to 82 in 0372617
arrow/util/logging.h
.Are these changes tested?
No test needed.
Are there any user-facing changes?
Probably not?
ARROW_DCHECK*
andDCHECK*
interchangeable? #43209