Lots of warnings when compiling with Visual Studio 2019

[obones]

I know my situation is a bit unusual, but I'm using Visual Studio 2019 to build this project. I'm using the solution file placed in the vs15 folder that was updated by the IDE.
As a base principle, I want zero messages from the compiler but when building the master branch, I get more than 300 warnings which I have placed in the following CSV file: warnings.csv

Here is a summary of the situation:

• 250 C4244/C4267/C4305 warnings: "conversion from XX to YY, possible loss of data"
  Most are from double to float and come from floating point literals
  Then there are conversions from double/float to an integer type
  And finally a dozen are from a large integer type to a narrower (one int64_t to int32_t for instance)

• 20 C4996 warnings: "The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name"

• 13 C6340 warnings: "Mismatch on sign: 'unsigned XX' passed as Param(N) when some signed type is required in call to 'fprintf'"
  Those ones are fixable via a change to the %d marker, but this might be Microsoft specific

• 12 C28251 warnings: "Inconsistent annotation for 'XXX': this instance has no annotations"
  I'm not sure what this is about

• 4 C6246 warnings: "Local declaration of 'XXX' hides declaration of the same name in outer scope"
  Not a big problem in and of itself, but I believe it would make the code clearer if this is fixed.

• 3 C4090 warnings: "different const qualifiers"
  These are oversights that I believe should be fixed for clarity's sake

The remaining warnings all revolve around static analysis which suggest that there might be some out of bounds reading, null de-referencing and the like.

What I have done on my local copy is to completely disable the following warnings: 4244; 4267; 4305; 4996
This removes most of them, but I'm not too happy with this, especially the ones related to data loss.

I can spend some time looking around for fixes on most of those warnings, but I would like your opinion before I start. For instance, for the ones related to floating points literals, should I change them all to use the f suffix, or should I change the variable types to all use double?
For conversions from floating point to integer types, should I use trunc or round?

Any opinions are most welcome.

[zuckschwerdt]

We have Visual Studio 2017 and Visual Studio 2019 in the GiHub Actions and see the same mess. We are using the CMake targets though.
The prebuild solution is configured with some specific options and paths which might not work too well. We a re thinking about removing that solution in favor of CMake. Is it a somewhat common and working approach to use CMake on Windows (the Actions builds work well, I wonder if developers are used to cmake)?

I had a look through the warnings (some time ago) and found most of it noise. "Fixing" seemed to be lots of workarounds to make the MSVC happy.

I wouldn't bother with the conversion warnings, maybe pick a few and check if there is really a problem we missed. Maybe look into it if that's the last thing coming up in warnings :)

Fixing the Posix stuff can only help portability, I guess it's just some #defines.

I didn't spot any format string type mismatches, those should be addressed.

Shadowing should be fixed, bad practice. When the project started there was no tight scoping, just 4 instances left sounds good ;)

As const is not a guarantee the problems need to be fixed, then at least the compiler can report some level of sanity there.

[merbanan]

Regarding the double to float warnings for example:
float samples_per_us = pulses->sample_rate / 1.0e6;
I think adding the f suffix here is the correct solution. It looks like there are 18 of those.

And the 4 C6246 warnings should definitive be fixed.

And C6340 seems looks like valid varnings also. The following warning:
https://github.com/merbanan/rtl_433/blob/master/src/devices/acurite.c#L675
should be solved with a change of the type declared here:
https://github.com/merbanan/rtl_433/blob/master/src/devices/acurite.c#L670

So fixes for those warnings will be accepted. But leave mongoose alone, those should be fixed upstream. Go for all the easy changes first then we can look into the other warnings that may hide real issues.

[zuckschwerdt]

I'm playing with GCC warnings and will perhaps fix some things. Hold off for a day or two ;)

[zuckschwerdt]

I've added some warnings and fixed those, 6c8af75.

E.g. Wmissing-prototypes showed missing static but needed some mock prototypes to silence the rest.
On the other hand Wbad-function-cast was not helpful, we do want to convert e.g. sqrt() to int.
And Wfloat-equal isn't useful either, we really do want to test for literal 0.0 (not a calculation).
The Wdocumentation showed a proper mistake, I'd want that, but it includes all the warnings from external headers :/

The intentional switch fallthrough is a handful, but it's only a single one we have and we'd really want a warning for that easy to make mistake.

#if (defined(__GNUC__) || defined(__clang__)) && __has_attribute(fallthrough)
            __attribute__((fallthrough));
#endif

The promotion and conversion warnings are not fun. So much noise.
I don't think that peppering the code with explicit casts will be a good thing, easy to make a mistake there.
E.g. I'd say afloat * aint does not look safer written as afloat * (float)aint

[obones]

The prebuild solution is configured with some specific options and paths which might not work too well. We a re thinking about removing that solution in favor of CMake. Is it a somewhat common and working approach to use CMake on Windows (the Actions builds work well, I wonder if developers are used to cmake)?

Well, I did have to change the prebuilt path to get a building environment, and the instructions aren't that clear but I'm very used to Visual Studio C++.
Using CMake is starting to become the norm but frankly it's still a pain because one has to "install" vcpkg, bootstrap it, install the appropriate libraries without messing up the target triplet. And even then libtrlsdr is missing from the vcpkg ports so one has to create it, finding the appropriate mirror to use at github (I have found at least 3, one of which I used to build librtlsdr in VC++)
If I find the courage, I'll create an issue here detailing what I have done so far, but right now, it does not compile yet.

The promotion and conversion warnings are not fun. So much noise.
I don't think that peppering the code with explicit casts will be a good thing, easy to make a mistake there.
E.g. I'd say afloat * aint does not look safer written as afloat * (float)aint

Yes, I agree that a hard cast is not nice, but the warning is not really about a missing cast but rather about a missing decision: should it be trunc, ceil or round?
Also, there are quite a few going from double to float where both operands are variables. There is some level of inconsistency here, and I believe the code should stick with either float or double. Coming from a world where precision is key, I'm more used to double but I believe most code in rtl_433 is using float so I'm suggesting banning double altogether except for very specific exceptions, if any.

I'm going to create various branches for the various "families" of warnings and issue the associated pull requests, keeping in mind suggestions made in this issue.

[gdt]

Seems like a lot of these have been resolved. But there are open linked PRs.

@obones My impression is that fixing UB is good, as long as we don't get icky, and that separate PRs for separate problems is probably best. You may want to freshen the unmerged PRs and/or open some new ones. I'm not sure this issue is useful, as if there is a PR, it should be examined, and if not, then I don't think anybody is going to go work on VS stuff because of the issue. But "there are warnings in the code that appear valid" is fair, so I don't object to it being open.

[obones]

I have rebased and adjusted the three branches linked to the three PRs mentioning this issue.

[gdt]

We have PRs so am going to close this. Feel free to file PRs to clean up things or to poke about the PRs.