Skip to content

Add BYN_WARN_UNUSED macro and apply to Name and IString - #8973

Merged
sbc100 merged 1 commit into
mainfrom
byn_warn_unused
Aug 6, 2026
Merged

Add BYN_WARN_UNUSED macro and apply to Name and IString#8973
sbc100 merged 1 commit into
mainfrom
byn_warn_unused

Conversation

@sbc100

@sbc100 sbc100 commented Aug 5, 2026

Copy link
Copy Markdown
Member

Define BYN_WARN_UNUSED using [[gnu::warn_unused]] for GCC/Clang and fallback for other compilers. Annotating IString and Name with this attribute allows compilers to report unused variable diagnostics for default-constructed instances regardless of non-trivial constructors.

Followup to #8972.

Once this lands we can extend this to other types to get better coverage.

@sbc100
sbc100 requested a review from a team as a code owner August 5, 2026 18:32
@sbc100
sbc100 requested review from aheejin and kripken and removed request for a team August 5, 2026 18:32
Comment thread src/support/istring.h
namespace wasm {

struct IString {
struct BYN_WARN_UNUSED IString {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This feels odd to me, to scatter these throughout the codebase. Is there no global flag we can set?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I mean its kind of like llvm/llvm-project#203084.

If there was a flag I guess it would be something like "-falways-assume-ctors-have-no-side-effects" but I'm pretty sure that is never always true so you wouldn't want it to be global.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, I see, so this is for the ctors? Can we annotate the ctors directly, then, not the entire class?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It seems like the way to do this is to annotate the class itself.

Annotating a C++ class with [[gnu::warn_unused]] tells the compiler to issue a -Wunused-variable warning if a variable of that type is instantiated but never explicitly used.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this documented somewhere? I can't seem to find GNU docs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The clang docs describe it: https://clang.llvm.org/docs/AttributeReference.html#warn-unused

"""
This attribute is available in both C and C++ language modes but is primarily useful in C++ for classes which have a non-trivial constructor or destructor but act as a value type rather than an RAII type.
"""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks... funny that it is a gnu thing but clang documents it 😄

Define `BYN_WARN_UNUSED` using `[[gnu::warn_unused]]` for GCC/Clang
and fallback for other compilers. Annotating `IString` and `Name` with
this attribute allows compilers to report unused variable diagnostics
for default-constructed instances regardless of non-trivial constructors.

Followup to #8972.

Once this lands we can extend this to other types to get better
coverage.
@sbc100

sbc100 commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

Funnily enough I couldn't fine any other classes in binaryen what had latent unused variables like the ones in this PR.

@sbc100
sbc100 enabled auto-merge (squash) August 6, 2026 19:47
@sbc100
sbc100 merged commit b97310a into main Aug 6, 2026
14 of 16 checks passed
@sbc100
sbc100 deleted the byn_warn_unused branch August 6, 2026 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants