Skip to content
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

Merged
merged 10 commits into from
Jul 16, 2024

Conversation

zanmato1984
Copy link
Collaborator

@zanmato1984 zanmato1984 commented Jul 14, 2024

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:

TEST(InternalHeaders, DCheckExposed) {
#ifdef DCHECK
FAIL() << "DCHECK should not be visible from Arrow public headers.";
#endif
}

So I'm making the following changes to DCHECK macro family.

What changes are included in this PR?

  1. Add lint rule for DCHECK usage in public headers;
  2. Cleanup exisiting DCHECK usages in public (probably non-public, but at least not named with internal) headers;
  3. Add ifdef protection for DCHECK definition like we did for ASSIGN_OR_RAISE (
    #ifndef ASSIGN_OR_RAISE
    #define ASSIGN_OR_RAISE(lhs, rhs) ARROW_ASSIGN_OR_RAISE(lhs, rhs)
    #endif
    ) and RETURN_NOT_OK (
    #ifndef RETURN_NOT_OK
    #define RETURN_NOT_OK(s) ARROW_RETURN_NOT_OK(s)
    #endif
    ), to not mess up user code that directly includes arrow/util/logging.h.
  4. Add comments as guideline.

Are these changes tested?

No test needed.

Are there any user-facing changes?

Probably not?

Copy link

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?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

Comment on lines -57 to -63
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
Copy link
Collaborator Author

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.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 14, 2024
@zanmato1984 zanmato1984 changed the title EXPERIMENT: [C++] Add lint for DCHECK in headers EXPERIMENT: [C++] Add lint for DCHECK in public headers Jul 14, 2024
@zanmato1984 zanmato1984 changed the title EXPERIMENT: [C++] Add lint for DCHECK in public headers GH-43209: [C++] Add lint for DCHECK in public headers Jul 14, 2024
@zanmato1984 zanmato1984 marked this pull request as ready for review July 14, 2024 17:35
Copy link

⚠️ GitHub issue #43209 has been automatically assigned in GitHub to PR creator.

@zanmato1984
Copy link
Collaborator Author

A small cleanup. @pitrou would you like to take a look? Thanks.

@wgtmac
Copy link
Member

wgtmac commented Jul 15, 2024

@github-actions crossbow submit -g cpp

This comment was marked as outdated.

@zanmato1984
Copy link
Collaborator Author

@github-actions crossbow submit -g cpp

This comment was marked as outdated.

Copy link
Member

@mapleFU mapleFU left a 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!

cpp/src/arrow/acero/unmaterialized_table.h Outdated Show resolved Hide resolved
cpp/src/arrow/util/bit_stream_utils.h Outdated Show resolved Hide resolved
cpp/src/arrow/util/vector.h Show resolved Hide resolved
@pitrou
Copy link
Member

pitrou commented Jul 15, 2024

@github-actions crossbow submit -g cpp

Copy link

Revision: cb0e3b1

Submitted crossbow builds: ursacomputing/crossbow @ actions-84164aa0b0

Task Status
test-alpine-linux-cpp GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-cuda-cpp GitHub Actions
test-debian-12-cpp-amd64 GitHub Actions
test-debian-12-cpp-i386 GitHub Actions
test-fedora-39-cpp GitHub Actions
test-ubuntu-20.04-cpp GitHub Actions
test-ubuntu-20.04-cpp-bundled GitHub Actions
test-ubuntu-20.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-20.04-cpp-thread-sanitizer GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-20 GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions

@pitrou
Copy link
Member

pitrou commented Jul 15, 2024

@zanmato1984 I think we should avoid @ing usernames in PR descriptions, because they end up in the final commit message and then can generate notification spam when people update their forks.

@zanmato1984
Copy link
Collaborator Author

@zanmato1984 I think we should avoid @ing usernames in PR descriptions, because they end up in the final commit message and then can generate notification spam when people update their forks.

Thanks for reminding. That's very thoughtful and I have updated the PR description. Appreciate that!

@pitrou pitrou merged commit 12f68fc into apache:main Jul 16, 2024
42 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Jul 16, 2024
@pitrou
Copy link
Member

pitrou commented Jul 16, 2024

Thanks a lot @zanmato1984 !

Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants