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.

Sign up for GitHub

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

Jump to bottom

ARROW-17803: [C++] Use [[nodiscard]] #14193

Merged
merged 2 commits into from
Sep 22, 2022
Merged

ARROW-17803: [C++] Use [[nodiscard]] #14193

merged 2 commits into from
Sep 22, 2022

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Sep 21, 2022

C++17 supports the standard attribute [[nodiscard]], use that instead of the clang-specific __attribute__((warn_unused_result)).

@pitrou
Copy link
Member Author

pitrou commented Sep 21, 2022

@js8544 For the record, it seems this version works around the GCC parser issue.

@pitrou

This comment was marked as outdated.

Sign in to view
@js8544
Copy link
Collaborator

js8544 commented Sep 21, 2022

@js8544 For the record, it seems this version works around the GCC parser issue.

Nice!

@pitrou
Copy link
Member Author

pitrou commented Sep 21, 2022

Linker issue which suggests the ARROW_FRIEND_EXPORT macro isn't right...
https://github.com/pitrou/arrow/actions/runs/3098917212/jobs/5017418044#step:7:1065

D:/a/_temp/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: src/arrow/CMakeFiles/arrow_testing_shared.dir/testing/gtest_util.cc.obj:gtest_util.cc:(.text$_ZN7testing13PrintToStringISt10shared_ptrIN5arrow6SchemaEEEENSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKT_[_ZN7testing13PrintToStringISt10shared_ptrIN5arrow6SchemaEEEENSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKT_]+0x69): undefined reference to `arrow::PrintTo(arrow::Schema const&, std::ostream*)'
D:/a/_temp/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: src/arrow/CMakeFiles/arrow_testing_shared.dir/compute/exec/test_util.cc.obj:test_util.cc:(.text+0x893): undefined reference to `arrow::compute::PrintTo(arrow::compute::Expression const&, std::ostream*)'
D:/a/_temp/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: src/arrow/CMakeFiles/arrow_testing_shared.dir/compute/exec/test_util.cc.obj:test_util.cc:(.text+0x988): undefined reference to `arrow::compute::PrintTo(arrow::compute::Expression const&, std::ostream*)'

@github-actions
Copy link

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

@pitrou

This comment was marked as outdated.

Sign in to view
@pitrou
Copy link
Member Author

pitrou commented Sep 22, 2022

Revision: 95c9694

Submitted crossbow builds: ursacomputing/crossbow @ nodiscard-2

Task Status
test-alpine-linux-cpp Github Actions
test-build-cpp-fuzz Github Actions
test-conda-cpp Github Actions
test-conda-cpp-valgrind Azure
test-debian-10-cpp-amd64 Github Actions
test-debian-10-cpp-i386 Github Actions
test-debian-11-cpp-amd64 Github Actions
test-debian-11-cpp-i386 Github Actions
test-fedora-35-cpp Github Actions
test-ubuntu-18.04-cpp Github Actions
test-ubuntu-18.04-cpp-release Github Actions
test-ubuntu-18.04-cpp-static Github Actions
test-ubuntu-20.04-cpp Github Actions
test-ubuntu-20.04-cpp-17 Github Actions
test-ubuntu-20.04-cpp-bundled Github Actions
test-ubuntu-20.04-cpp-thread-sanitizer Github Actions
test-ubuntu-22.04-cpp Github Actions

lidavidm
lidavidm reviewed Sep 22, 2022
View reviewed changes
cpp/src/arrow/util/visibility.h Outdated Show resolved Hide resolved
cpp/src/arrow/util/future.h Outdated Show resolved Hide resolved
@pitrou
ARROW-17803: [C++] Use [[nodiscard]]
aaf3490 
C++17 supports the standard attribute `[[nodiscard]]`, use that instead of the clang-specific `__attribute__((warn_unused_result))`.
@pitrou pitrou merged commit 5256e43 into apache:master Sep 22, 2022
@pitrou pitrou deleted the ARROW-17803-no-discard branch September 22, 2022 14:59
@ursabot
Copy link

ursabot commented Sep 23, 2022

Benchmark runs are scheduled for baseline = f477a97 and contender = 5256e43. 5256e43 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️1.09% ⬆️0.14%] test-mac-arm
[Failed ⬇️1.37% ⬆️0.27%] ursa-i9-9960x
[Finished ⬇️0.25% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 5256e434 ec2-t3-xlarge-us-east-2
[Failed] 5256e434 test-mac-arm
[Failed] 5256e434 ursa-i9-9960x
[Finished] 5256e434 ursa-thinkcentre-m75q
[Finished] f477a972 ec2-t3-xlarge-us-east-2
[Failed] f477a972 test-mac-arm
[Failed] f477a972 ursa-i9-9960x
[Finished] f477a972 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented Sep 23, 2022

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
@pitrou @zagto
ARROW-17803: [C++] Use [[nodiscard]] (apache#14193)
f0c6e36 
C++17 supports the standard attribute `[[nodiscard]]`, use that instead of the clang-specific `__attribute__((warn_unused_result))`.

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
@pitrou @fatemehp
ARROW-17803: [C++] Use [[nodiscard]] (apache#14193)
9e5cad6 
C++17 supports the standard attribute `[[nodiscard]]`, use that instead of the clang-specific `__attribute__((warn_unused_result))`.

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@asfimport asfimport mentioned this pull request Sep 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lidavidm lidavidm lidavidm approved these changes

@bkietz bkietz Awaiting requested review from bkietz

Assignees
No one assigned
Labels
Component: C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pitrou @js8544 @ursabot @lidavidm