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

MINOR: [C++] Fix compiler warnings in Clang #43278

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

jaepil
Copy link

@jaepil jaepil commented Jul 16, 2024

This PR fixes compiler warnings in Clang.

❯ ninja clean && ninja
[1/1] Cleaning all built files...
Cleaning... 0 files.
[289/391] Building CXX object src/arrow/acero/CMakeFiles/arrow_acero_objlib.dir/asof_join_node.cc.o
/Users/jaepil/work/appspand/deps/arrow/cpp/src/arrow/acero/asof_join_node.cc:782:17: warning: private field 'node_' is not used [-Wunused-private-field]
  782 |   AsofJoinNode* node_;
      |                 ^
/Users/jaepil/work/appspand/deps/arrow/cpp/src/arrow/acero/asof_join_node.cc:784:10: warning: private field 'index_' is not used [-Wunused-private-field]
  784 |   size_t index_;
      |          ^
2 warnings generated.
[391/391] Linking CXX static library release/libarrow_substrait.a

@kou
Copy link
Member

kou commented Jul 16, 2024

Could you open a new issue for this?
This is not a MINOR change. See also our MINOR definition: https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes

Could you use our PR template instead of removing it entirely?

Could you show how to reproduce this?
What is your Clang version? How did you run cmake?

@jaepil
Copy link
Author

jaepil commented Jul 17, 2024

Could you open a new issue for this? This is not a MINOR change. See also our MINOR definition: https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes

Could you use our PR template instead of removing it entirely?

Could you show how to reproduce this? What is your Clang version? How did you run cmake?

No problem. I should have read the CONTRIBUTING.md more carefully. I will update the PR description and create an issue later today.

Thanks for the feedback!

Copy link
Collaborator

@zanmato1984 zanmato1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we do the same thing to these two members as what we did to MemoStore, as the changes I proposed?

Of course, this requires some other cleanup too.

Thank you.

@@ -779,9 +779,9 @@ class InputState {
// Hasher for key elements
mutable KeyHasher* key_hasher_;
// Owning node
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Owning node
#ifndef NDEBUG
// Owning node

@@ -779,9 +779,9 @@ class InputState {
// Hasher for key elements
mutable KeyHasher* key_hasher_;
// Owning node
AsofJoinNode* node_;
[[maybe_unused]] AsofJoinNode* node_;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[[maybe_unused]] AsofJoinNode* node_;
AsofJoinNode* node_;

// Index of this input
size_t index_;
[[maybe_unused]] size_t index_;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[[maybe_unused]] size_t index_;
size_t index_;
#endif

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 17, 2024
@mapleFU
Copy link
Member

mapleFU commented Oct 31, 2024

@zanmato1984 so these two members are only used in debug mode?

@zanmato1984
Copy link
Collaborator

@zanmato1984 so these two members are only used in debug mode?

Yes, I think so.

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

Successfully merging this pull request may close these issues.

4 participants