search

RPGHacker / asar

(Now) official repository of the SNES assembler Asar, originally created by Alcaro
Other
199 stars 42 forks source link

Reduce warnings #249

Closed Atari2 closed 2 years ago

Atari2 commented 2 years ago

This patch makes the number of warnings on the MSVC build of asar go from 2.8k to ~160.

The main reducer is going from /Wall to /W3. Since /Wall is like clang's -Weverything, it tends to be overly noisy and warn about unimportant things, /W3 is much less noisy while still warning about the more important stuff.

Additionally I turned off warnings 4244 and 5045, 4244 because it was causing ~350 warnings alone and as such I doubt they'd all get fixed manually one by one and 5045 because it's a pretty useless warning in asar's case.

To remove another ~90 warnings given by 9025 I made use of CMake's MSVC_RUNTIME_LIBRARY target property. This is because if you manually specify /MT or /MD on the target_compile_options() CMake will just append them resulting in both /MT and /MD in the command line, because [from CMake's docs]

If this property is not set then CMake uses the default value MultiThreaded$<$:Debug>DLL to select a MSVC runtime library.

obviously the compiler will ignore the first and only use the second one but it will still print a warning about it. Using that specific target property avoids the duplicate command line flag and thus the warning. Note that this is why I had to add

cmake_policy(SET CMP0091 NEW).

RPGHacker commented 2 years ago

4244 actually sounds like it could still be a useful warning in some cases. I could definitely see that preventing a few bugs that you might be searching for quite a while otherwise. Though I'm not sure how often that's actually thrown in Asar, so not sure if it's feasible to re-enable?

Atari2 commented 2 years ago

4244 is certainly useful in some cases but when it appears image this many times, it's virtually impossible to distinguish between the times it's actually a concern vs the times it's harmless. Unless someone is willing to go through and check every single warning. That's why I decided to turn it off instead. Fyi, most of them look like this image

RPGHacker commented 2 years ago

Yeah, I figured... I guess that's something for future me to clean up (aka: something that will never be cleaned up).