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

[windows] Swig is building a bad "SoapySDRPYTHON_wrap.cxx" causing C4101 warning in MSVC for unreferenced local variable #449

Open
eabase opened this issue Feb 19, 2025 · 6 comments

Comments

@eabase
Copy link

eabase commented Feb 19, 2025

Compiling SoapySDR on windows, we get 1000's of warning error for a silly error, due to not sanitizing a catch() statement, in the build file: SoapySDRPYTHON_wrap.cxx from: swig\python\python3\CMakeFiles\_SoapySDR3.dir\.

This causes:

"warning C4101: 'e': unreferenced local variable"

Which comes from the following repeated catch statement:
catch (const Swig::DirectorException &e) { SWIG_fail;}

This catch condition/statement need to be replaced by:
catch([[maybe_unused]] const Swig::DirectorException &e)

Because e is not used and also can not be disabled by #pragma warning(disable:4101) as that only works on a function basis.

@eabase
Copy link
Author

eabase commented Feb 19, 2025

As I am not enough familiar with any of these tools, I am not able to help further than finding the statements within the Swig generated file, using bash:

grep -n --color=always -H -R -i -E "catch \(const Swig::DirectorException \&e\)" ./

How is this generated by Swig?
From what exactly?
Is this a Swig issue? (I doubt it.)

@zuckschwerdt
Copy link
Member

We should maybe compile wirh /W3 on MSVC, similar to for GCC/Clang would be
set_source_files_properties(${swig_generated_file_fullname} PROPERTIES COMPILE_FLAGS "-Wno-unused-parameter")
in https://github.com/pothosware/SoapySDR/blob/master/swig/python/CMakeLists.txt#L110

@zuckschwerdt
Copy link
Member

Does this work? Add in https://github.com/pothosware/SoapySDR/blob/master/swig/python/CMakeLists.txt#L110

    if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC")
        set_source_files_properties(${swig_generated_file_fullname} PROPERTIES COMPILE_FLAGS "/W3")
    endif()

@eabase
Copy link
Author

eabase commented Feb 20, 2025

Hi Christian,
I'm a bit busy right now, so will get back to you with some tests.

As a quick comment, I'm not a big fan of having to "hide" compiler warnings. They are there for a reason, and that reason usually breaks eventually. In addition, users should not have to resort to tweak compiler warnings. As of today there are already 2 of those in use; -Wno-dev and -Wno-deprecated. So lets try to avoid having to add another one...

Also, the fix for above may be just finding and replacing them in:

./swig/csharp/swig/SoapySDR.i:49
./swig/python/SoapySDR.in.i:90

BTW. I've also started looking into building/installing from MSYS/MINGW64. As out-of-the-box it runs and detects after also installing "recommended" mingw-w64-x86_64-soapyrtlsdr, this is essential but was never mentioned anywhere (I could find) here. I have no idea what is the equivalent Windows package or build to make, to do that...

@zuckschwerdt
Copy link
Member

I'm also seeing unused parameter 'self' for the PyObject *self in the wrapper functions, which should be addressed too.
The offending lines are generated and it would be nice if Swig could determine that a SWIGUNUSEDPARM() is needed. Not sure how that works or why is not picking that up.

Excluding the unused-parameter warning to focus on other warning is maybe the best we can do here.

@eabase
Copy link
Author

eabase commented Feb 23, 2025

I'm looking at this again...
Seem to be many options.

First of all I want to ask:

Is there a reason why we are using compiler C++11 instead of C++17?

From:
https://github.com/pothosware/SoapySDR/blob/master/CMakeLists.txt#L23-L24

#C++11 is a required language feature for this project
set(CMAKE_CXX_STANDARD 11)

I managed to rid all the C4101 by using the change in OP and applying compiler to set(CMAKE_CXX_STANDARD 17).


The question is also if this is an unused parameter or any of the others?
I thought it could be:

-Wunused-const-variable
-Wunused-variable

It may be also be possible to use:

<PropertyGroup>
  <NoWarn>$(NoWarn);MSB3253</NoWarn>
</PropertyGroup>

See:
https://stackoverflow.com/a/63512122/
https://stackoverflow.com/questions/41205725/how-to-disable-specific-warning-inherited-from-parent-in-visual-studio


Also, I don't think is a good idea to reduce (from /W4 to /W3) the warnings using compiler options. Mainly for 2 reasons:

  1. It's too general and would obfuscate more serious errors. We should use as minimal error disables, as possible.
  2. Compiler options can be overridden within the build framework by multiple settings, making it unclear what is really applied and where.

https://learn.microsoft.com/en-us/cpp/build/reference/compiler-option-warning-level?view=msvc-170

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

No branches or pull requests

2 participants