Repository navigation
Turn on GCC's -Wredundant-tags, and drop the struct and enum keywords - #253
Merged
Merged
Conversation
GCC's -Wredundant-tags is on. It reports a struct, union, or enum keyword in front of a type's name where C++ doesn't need it, as in struct ibv_pd *pd. Without the keyword, naming a type that hasn't been declared is a compile error. With it, the name quietly declares a new type in the enclosing namespace (hipObj::ibv_pd, not libibverbs' ::ibv_pd). Clang has no equivalent, so only GCC builds check it. - The keywords GCC reported are gone from the library, the tests, the test server, and the s3_curl_ops example, including ibv-core.h. The layout test still checks ibv-core.h against <infiniband/verbs.h>. Where a keyword is the type's first mention, as for ibv_device in ibv_device_ops, it stays. - ibv-layout-list.h is also included by the C file ibv-layout-real.c, and C needs the keyword, so each entry now passes it as its own argument: HIPOBJ_IBV_MEMBER(struct, ibv_gid_entry, gid). ibv-layout-real.c puts it in front of the name, and test-ibv-layout.cpp only uses it in its failure messages. - AGENTS.md's IWYU notes are updated. Without the keywords, IWYU suggests replacing ibv-core.h with forward declarations of the verbs types in src/buffer.h and src/common/ibv-wrapper.h. The include stays, since ibv-core.h declares the types in the global namespace, as libibverbs does. Co-Authored-By: Claude <noreply@anthropic.com>
derobins
requested review from
akolliasAMD,
gaoikawa,
jordan-turbofish,
kurtmcmillan,
riley-dixon,
sbates130272 and
zbyrne
as code owners
October 17, 2026 23:32
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The mechanical cleanup preserves type identities and C layout-test compatibility, with no blocking defects identified.
No open issues 🎉
What changed in this PR
Enables GCC’s redundant-tag warning to help prevent accidental type declarations in hipObject’s C++ code.
Changes:
- Removes redundant type keywords from library code, tests, and the curl example.
- Adapts shared layout-test macros to preserve C compatibility.
- Updates IWYU guidance on retaining verbs headers.
| File | Description |
|---|---|
| test/unit/test-transport.cpp | Simplifies transport test types. |
| test/unit/test-span.cpp | Simplifies device pointer types. |
| test/unit/test-seams.cpp | Simplifies fake verbs types. |
| test/unit/test-s3-curl-ops.cpp | Removes redundant socket tags. |
| test/unit/test-loaded-verbs.cpp | Simplifies verbs forwarding tests. |
| test/unit/test-ibv-layout.cpp | Omits tags in C++ macro expansions. |
| test/unit/test-ibv-dmabuf.cpp | Simplifies memory registration types. |
| test/unit/test-connection-pool.cpp | Simplifies connection test types. |
| test/unit/test-buffer-registration.cpp | Simplifies registration test types. |
| test/unit/test-api-exceptions.cpp | Simplifies exception-test overrides. |
| test/unit/ibv-layout-real.c | Preserves tags in C macro expansions. |
| test/unit/ibv-layout-list.h | Separates keywords from type names. |
| test/unit/fake-device.h | Simplifies fake verbs declarations. |
| test/unit/fake-device.cpp | Simplifies fake verbs definitions. |
| test/integration/rdma-test-server/rdma_server.cpp | Simplifies RDMA helper signatures. |
| src/rdma/vendor-ops.h | Simplifies vendor operation declarations. |
| src/rdma/transport.h | Simplifies transport declarations. |
| src/rdma/transport.cpp | Simplifies transport type usage. |
| src/rdma/ionic/ionic-backend.cpp | Simplifies Ionic helper signature. |
| src/rdma/bnxt/bnxt-backend.cpp | Simplifies Broadcom helper signature. |
| src/common/ibv-wrapper.h | Simplifies verbs interface declarations. |
| src/common/ibv-wrapper.cpp | Simplifies verbs wrapper definitions. |
| src/common/ibv-ptr.h | Simplifies smart pointer types. |
| src/common/ibv-core.h | Removes redundant tags from verbs types. |
| src/buffer.h | Simplifies buffer registration declarations. |
| src/buffer.cpp | Simplifies buffer registration definitions. |
| examples/s3_curl_ops.cpp | Simplifies curl list types. |
| cmake/HIPOBJGNUCompilerOptions.cmake | Enables -Wredundant-tags. |
| AGENTS.md | Clarifies IWYU header guidance. |
Review effort: Balanced
Contributor
Security scan found no critical or high findings on lines changed by this pull requestView the security scan run for complete reports. “New” means the finding is located in a changed file on an added or modified line; it is not a base-versus-head findings delta. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GCC's -Wredundant-tags is on. It reports a struct, union, or enum keyword in front of a type's name where C++ doesn't need it, as in struct ibv_pd *pd. Without the keyword, naming a type that hasn't been declared is a compile error. With it, the name quietly declares a new type in the enclosing namespace (hipObj::ibv_pd, not libibverbs' ::ibv_pd). Clang has no equivalent, so only GCC builds check it.