SoapySDR icon indicating copy to clipboard operation
SoapySDR copied to clipboard

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

Open eabase opened this issue 10 months ago • 6 comments

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 avatar Feb 19 '25 04:02 eabase

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.)

  • https://github.com/swig/swig/issues

eabase avatar Feb 19 '25 04:02 eabase

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 avatar Feb 19 '25 11:02 zuckschwerdt

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()

zuckschwerdt avatar Feb 19 '25 11:02 zuckschwerdt

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...

eabase avatar Feb 20 '25 13:02 eabase

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.

zuckschwerdt avatar Feb 20 '25 13:02 zuckschwerdt

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

eabase avatar Feb 23 '25 13:02 eabase