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

Clean up compiler warnings (Mark unused variables with '[[maybe_unused]]') #2132

Closed

Conversation

jtkmckenna
Copy link

As part of #2006, warnings generated by -Wall and -Wextra will need to be fixed. The tallest nail gets the hammer, I see at least 90 'unused-parameter' warnings on MacOS with the default build options.

Here I make a minimal pull request fixing one file to make discussion easy:

src/presolve/HighsPostsolveStack.h:788:47: warning: unused parameter 'flagRow' [-Wunused-parameter]
src/presolve/HighsPostsolveStack.h:789:47: warning: unused parameter 'flagCol' [-Wunused-parameter]

I see the minimum C++ standard is C++11, I thought this would preclude some more modern features, but compilers seem to support some

I see two ways of fixing this warning (that don't change the inputs to the function):

Option 1: Use [[maybe_unused]]

int example_function([[maybe_unused]] int a)
{
    return 0;
}

This is a 'modern' feature, while officially a C++17 feature, my tests show most compilers seem to understand it (even when set to C++11)

https://godbolt.org/z/qdPvo6Mdc

Testing shows that the minimum compiler version (that build this test function in C++11)
gcc 7.1
clang 3.9.0
msvc v19.21 VS16.1*

*msvc throws a warning that [[maybe_unused]] is ignored

I am very interested to see if your CI actions are happy with this syntax.

Optional 2: Comment out the variable name

int example_function(int /* a */)
{
    return 0;
}

This is perhaps a more old fashioned fix, the major disadvantage is that /* */ doesn't nest. So it becomes annoying to comment out an entire function.

I look forward to hearing your thoughts

@jtkmckenna jtkmckenna changed the title Clean up compiler warnings (Make unused variables with '[[maybe_unused]]') Clean up compiler warnings (Mark unused variables with '[[maybe_unused]]') Jan 14, 2025
@jajhall jajhall requested a review from galabovaa January 14, 2025 15:39
@jajhall jajhall changed the base branch from master to latest January 14, 2025 15:39
@galabovaa
Copy link
Contributor

Yes it's not ideal to get the warnings and we mostly surpress them in the default options. While we do intend, of course, to fix them at some point, there are higher priority tasks at the moment.

We can surpress those from CMake. Is there a way for you to surpress them on your side, as well? Things like unused variables are not concerning about performance so are OK to ignore for the moment

@jtkmckenna
Copy link
Author

jtkmckenna commented Jan 14, 2025

I agree these warnings have no real impact on performance, however this one function is uniquely exposed since it lives in a header file.

Your default build settings didn't raise warnings. So you wont need to change the build on your side (I modified the CMake build to turn the warnings while I fixed them). On my side its a little harder...

The warning is a little bit of a headache as it raises warnings in a personal project when simply including #include "Highs.h". Unfortunately I cannot suppress compiler warnings in CMake on a per file basis since header files aren't directly compiled.

If the implementation of undoUntil where moved into a cpp file, then an external project wouldn't see the variable being unused.

@jajhall
Copy link
Member

jajhall commented Jan 15, 2025

Sorry, but we're not allowing this C++17 feature just to silence your compiler warnings.

The offending parameters have been removed in #2133

@jajhall jajhall closed this Jan 15, 2025
@jtkmckenna
Copy link
Author

Great! Thank you for the swift review

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.

3 participants