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

Don't ignore GCC-specific warning under Clang #1557

Merged
merged 1 commit into from
May 14, 2024

Conversation

aaronmondal
Copy link
Contributor

Clang doesn't know -Wnon-template-friend.

@aaronmondal aaronmondal requested a review from a team as a code owner May 8, 2024 17:05
@aaronmondal aaronmondal requested review from harrism and miscco May 8, 2024 17:05
Copy link

copy-pr-bot bot commented May 8, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added the cpp Pertains to C++ code label May 8, 2024
@harrism harrism added bug Something isn't working non-breaking Non-breaking change labels May 9, 2024
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

@aaronmondal thanks for this contribution, much appreciated! I'd like @miscco to also approve since I think he added this GCC ignore.

@wence-
Copy link
Contributor

wence- commented May 10, 2024

/ok to test

Clang doesn't know `-Wnon-template-friend`.
@wence-
Copy link
Contributor

wence- commented May 10, 2024

/ok to tset

@aaronmondal
Copy link
Contributor Author

/ok to test

@wence-
Copy link
Contributor

wence- commented May 13, 2024

/ok to test

@aaronmondal you don't have the permissions to do that

@wence-
Copy link
Contributor

wence- commented May 13, 2024

/ok to test

Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

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

Thanks I hate it,

we have a special macro for that because it is so annoying

@wence-
Copy link
Contributor

wence- commented May 14, 2024

/merge

@wence-
Copy link
Contributor

wence- commented May 14, 2024

Thanks!

@rapids-bot rapids-bot bot merged commit 32cd537 into rapidsai:branch-24.06 May 14, 2024
57 checks passed
@@ -75,18 +75,19 @@ struct maybe_remove_property<PoolResource,
Upstream,
Property,
cuda::std::enable_if_t<!cuda::has_property<Upstream, Property>>> {
#ifdef __GNUC__ // GCC warns about compatibility issues with pre ISO C++ code
#if defined(__GNUC__) && !defined(__clang__) // GCC warns about compatibility
Copy link
Member

Choose a reason for hiding this comment

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

Just thought of this: Under what circumstances are __GNUC__ and __clang__ both defined? They must be or we wouldn't need this logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • __GNUC__ means "compatibility with GNU C". This is defined by both GCC and Clang.
  • __clang__ means Clang. I'd guess that this is also defined by clang derivatives such as HIPCC and DPC++. But it's not defined by GCC.
  • AFAIK GCC doesn't have a similar macro. I.e. there is no __gcc__ macro.

Under what circumstances are __GNUC__ and __clang__ both defined?

When you run a compilation with clang this is the case.

The "-Wnon-template-friend" exists in GCC, but not in Clang. With the way things are now we're essentially saying: If you're a GNU C compatible compiler, but you're not Clang (or a Clang derivative where we can expect warning flags to be reasonably similar to clang), then add -Wnon-template-friend to the ignorelist.

Before this patch Clang triggered warnings like warning: ignoring unknown warning which spammed the logs.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the informative answer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cpp Pertains to C++ code non-breaking Non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants