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

Suppress unused variable warning in helper_thread.c #1789

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

r-barnes
Copy link

Allows -Wunused-but-set-variable to pass in opt mode. Adds what appears to be a missing assert.

Please confirm that your commit message(s) follow these guidelines:

  1. First line is a commit title, a descriptive one-liner for the change
  2. Empty second line
  3. Commit message body that explains why the change is useful. Break lines that
    aren't something like a URL at 72-74 chars.
  4. Empty line
  5. Signed-off-by: Real Name real@email.com

Reminders:

  1. If you modify struct thread_options, also make corresponding changes in
    cconv.c and bump FIO_SERVER_VER in server.h
  2. If you change the ioengine interface (hooks, flags, etc), remember to bump
    FIO_IOOPS_VERSION in ioengines.h.

Allows `-Wunused-but-set-variable` to pass in opt mode. Adds what appears to be a missing assert.
helper_thread.c Outdated
@@ -110,10 +110,12 @@ static void block_signals(void)
sigset_t sigmask;

int ret;
(void)ret; // Suppress unused variable warning
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this line necessary with the assert added below?

Copy link
Author

Choose a reason for hiding this comment

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

It is. A more comprehensive take would be:

#if defined(NDEBUG)
#define USED_FOR_DEBUG(x) (void)0;
#else
#define USED_FOR_DEBUG(x) (void)x;
#endif

That way the warning is suppressed during debug mode, but not during opt mode.

But this seems simpler for a smaller codebase.

@vincentkfu
Copy link
Collaborator

I've built with ./configure --extra-cflags=Wall and confirmed that CONFIG_PTHREAD_SIGMASK is defined but cannot trigger the warning that you report.

Our CI builds with -Werror and this warning does not appear.

What did you do to trigger this warning?

@r-barnes
Copy link
Author

@vincentkfu Our builds use a different build system with LLVM-15 with -Wunused-but-set-variable enabled, and -Werror. That triggers a build failure for the instance in question.

@vincentkfu
Copy link
Collaborator

Ok. The convention in our codebase is to use /* ... */ for comments. Please make this change to your patch.

@r-barnes
Copy link
Author

@vincentkfu - Thanks! I've made the change. My apologies for not noticing your convention the first time.

@vincentkfu
Copy link
Collaborator

These are small changes but I can't let them go in this way.

How about this:

Patch 1: add missing assert only
Patch 2: add (void) ret; to suppress unused variable warning

Please make sure the commit messages for each patch accurately describe the purpose of the change.

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