search

RobertBeckebans / RBDOOM-3-BFG

Doom 3 BFG Edition source port with updated DX12 / Vulkan renderer and modern game engine features
https://www.moddb.com/mods/rbdoom-3-bfg
GNU General Public License v3.0
1.38k stars 247 forks source link

Fix gcc/clang & MSVC warnings for all platforms #794

Closed SRSaunders closed 10 months ago

SRSaunders commented 10 months ago

This is a relatively simple, but medium-sized pull request that solves most compiler warnings. It was motivated by me moving my dev machine to macOS Ventura with Xcode 14 / Apple clang 14.0.3 which is based on LLVM 15. This resulted in tons of new compiler warnings, fortunately concentrated on a small number of things. After solving those, I decided to attack linux gcc and clang 15 warnings and finally Visual Studio 2019's MSVC warnings. Solutions are in three broad categories:

  1. Fix deprecations in core RBDoom3BFG and rapidjson, e.g. sprintf() and vsprintf(), std::iterator
  2. Fix type mismatches in core RBDoom3BFG and a few third-party source libs, e.g. libbinkdec, tinyexr
  3. Apply targeted warning suppression where warranted, e.g. heap inline new/delete, int 64 to 32, switch statements with default but no case, and specific warnings for some third-party source libs (jpeg, png, oggvorbis, zlib, minizip) where I didn't feel the effort was worth it, or could be solved by up-versioning libs in the future.

as well as fixes for several specific areas:

  1. Make Classic Doom implicit uint math operations more explicit, and in the process eliminate warnings
  2. Fix 64-bit vs 32-bit return type handling in win_syscon.cpp by using LRESULT and SetWindowLongPtr()
  3. Minimize header include path interaction between system libs vs bundled libs (i.e. libjpeg, libpng and zlib, rapidjson). I'm not sure if this capability is still important in the nvrhi world, but nonetheless it works better now.

One of the main issues involving many files was the unexpected deprecation of sprintf() and vsprintf() by the Apple clang 14.0.3 compiler. This seems a little aggressive on Apple's part, but I guess is the right thing to do for buffer security. Fortunately there is a relatively easy solution with idStr::snPrintf() and idStr::vsnPrintf() which are cross platform portable implementations with consistent length, null termination and error reporting behaviour. As part of this fix, I updated the implementation of idStr::snPrintf() to be based on idStr::vsnPrintf() without need for an internal buffer versus the old implementation using vsprintf() with a large 32K internal buffer. This should make things a little more memory efficient for all platforms.

RobertBeckebans commented 10 months ago

I had to look over this twice but I like that almost everything is now based on idStr::vsnPrintf(). It simplifies the implementation and also the changes in the filesystem seem to be save. I'm not sure about the UINT_MAX changes but I trust you that those are correct too.

SRSaunders commented 10 months ago

Yes, the Doom Classic negative uint math was using a compiler trick to calculate complementary angles which seems brittle to me, and causes warnings on newer compilers (e.g. clang). Adding UINT_MAX + 1 is like adding 0 to the original calculation, but when done in proper order (i.e. UINT_MAX - x + 1) results in a calculation that is more understandable and does not cause warnings with modern compilers.