libgme / game-music-emu

Blargg's video game music emulation library, which allows audio applications to easily add playback support for the music of many classic video game consoles.
GNU Lesser General Public License v2.1
59 stars 12 forks source link

Use `nullptr` for all null pointer constants #79

Open myQwil opened 8 months ago

myQwil commented 8 months ago

Also added -Wzero-as-null-pointer-constant to cxx flags.

Ensuring that only nullptr is used with pointer types can prevent accidental assignments or comparisons with non-pointer types, reduce the risk of subtle bugs, and improve code reliability. It also helps to avoid ambiguity and improve clarity and intent.

Wohlstand commented 8 months ago

Be careful! Don't add this argument directly, add at least a macro that checks it and adds when compiler do really has it.

Here is an example of macro: https://github.com/WohlSoft/AudioCodecs/blob/master/audio_codec_common.cmake#L17-L22 And the example on how to use it:

ac_add_cxx_warning_flag(zero-as-null-pointer-constant ZERO_AS_NULLPTR_CONSTANT)

Anyway, please rename ac_add_cxx_warning_flag into gme_add_cxx_warning_flag.

Wohlstand commented 8 months ago

Looks much better now! Soon I'll take more deeper look and merge.

sezero commented 8 months ago

There seems to be misses? I get the following from my compiler:

/tmp/libgme/gme/gme.cpp: In function 'const char* gme_track_info(const Music_Emu*, gme_info_t**, int)':
/tmp/libgme/gme/gme.cpp:304:7: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  *out = NULL;
       ^
/tmp/libgme/gme/Nes_Apu.cpp: In constructor 'Nes_Apu::Nes_Apu()':
/tmp/libgme/gme/Nes_Apu.cpp:26:17: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  dmc.prg_reader = NULL;
                 ^
/tmp/libgme/gme/Nes_Apu.cpp:27:16: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  irq_notifier_ = NULL;
                ^
/tmp/libgme/gme/Nes_Apu.cpp:35:15: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  output( NULL );
               ^
In file included from /tmp/libgme/gme/nes_cpu_io.h:6:0,
                 from /tmp/libgme/gme/Nes_Cpu.cpp:28:
/tmp/libgme/gme/Nes_Fds_Apu.h: In constructor 'Nes_Fds_Apu::Nes_Fds_Apu()':
/tmp/libgme/gme/Nes_Fds_Apu.h:127:22: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  osc_output( 0, NULL );
                      ^
In file included from /tmp/libgme/gme/Nes_Fme7_Apu.cpp:3:0:
/tmp/libgme/gme/Nes_Fme7_Apu.h: In constructor 'Nes_Fme7_Apu::Nes_Fme7_Apu()':
/tmp/libgme/gme/Nes_Fme7_Apu.h:89:15: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  output( NULL );
               ^
/tmp/libgme/gme/Nes_Namco_Apu.cpp: In constructor 'Nes_Namco_Apu::Nes_Namco_Apu()':
/tmp/libgme/gme/Nes_Namco_Apu.cpp:20:15: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  output( NULL );
               ^
/tmp/libgme/gme/Nes_Vrc6_Apu.cpp: In constructor 'Nes_Vrc6_Apu::Nes_Vrc6_Apu()':
/tmp/libgme/gme/Nes_Vrc6_Apu.cpp:20:15: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  output( NULL );
               ^
In file included from /tmp/libgme/gme/Nes_Fds_Apu.cpp:3:0:
/tmp/libgme/gme/Nes_Fds_Apu.h: In constructor 'Nes_Fds_Apu::Nes_Fds_Apu()':
/tmp/libgme/gme/Nes_Fds_Apu.h:127:22: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  osc_output( 0, NULL );
                      ^
In file included from /tmp/libgme/gme/Nsf_Emu.cpp:13:0:
/tmp/libgme/gme/Nes_Fme7_Apu.h: In constructor 'Nes_Fme7_Apu::Nes_Fme7_Apu()':
/tmp/libgme/gme/Nes_Fme7_Apu.h:89:15: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  output( NULL );
               ^
In file included from /tmp/libgme/gme/Nsf_Emu.cpp:14:0:
/tmp/libgme/gme/Nes_Fds_Apu.h: In constructor 'Nes_Fds_Apu::Nes_Fds_Apu()':
/tmp/libgme/gme/Nes_Fds_Apu.h:127:22: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  osc_output( 0, NULL );
                      ^
In file included from /tmp/libgme/gme/Vgm_Emu.h:7:0,
                 from /tmp/libgme/gme/Vgm_Emu.cpp:3:
/tmp/libgme/gme/Vgm_Emu_Impl.h: In instantiation of 'Ym_Emu<Emu>::Ym_Emu() [with Emu = Ym2612_Nuked_Emu]':
/tmp/libgme/gme/Vgm_Emu_Impl.h:27:7:   required from here
/tmp/libgme/gme/Vgm_Emu_Impl.h:20:74: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  Ym_Emu()                        : last_time( disabled_time ), out( NULL ) { }
                                                                          ^
/tmp/libgme/gme/Vgm_Emu_Impl.h: In instantiation of 'Ym_Emu<Emu>::Ym_Emu() [with Emu = Ym2413_Emu]':
/tmp/libgme/gme/Vgm_Emu_Impl.h:27:7:   required from here
/tmp/libgme/gme/Vgm_Emu_Impl.h:20:74: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
/tmp/libgme/player/Music_Player.cpp: In constructor 'Music_Player::Music_Player()':
/tmp/libgme/player/Music_Player.cpp:49:14: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  track_info_ = NULL;
              ^
/tmp/libgme/player/Music_Player.cpp: In member function 'void Music_Player::stop()':
/tmp/libgme/player/Music_Player.cpp:68:7: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  emu_ = NULL;
       ^
/tmp/libgme/player/Music_Player.cpp: In member function 'const char* Music_Player::start_track(int)':
/tmp/libgme/player/Music_Player.cpp:140:15: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
   track_info_ = NULL;
               ^

P.S.: @Wohlstand: Can you apply the following to demo/CMakeLists.txt? demo/basics_multi.c seems to need it.

diff --git a/demo/CMakeLists.txt b/demo/CMakeLists.txt
index b4453f7..82991ce 100644
--- a/demo/CMakeLists.txt
+++ b/demo/CMakeLists.txt
@@ -6,4 +6,6 @@ include_directories(${CMAKE_SOURCE_DIR}/gme ${CMAKE_SOURCE_DIR})
 link_directories(${CMAKE_BINARY_DIR}/gme)

+set (CMAKE_C_STANDARD 99)
+
 add_executable(demo Wave_Writer.cpp basics.c)
myQwil commented 8 months ago

@sezero Okay, I'm getting the same warnings as you now. I needed to build with clang. For some reason, GNU isn't picking up all of them.

It seems to have something to do with how NULL is being interpreted.

sezero commented 8 months ago

Well, the gcc I used is 4.9 which is pretty old. And I believe that the warning should actually be really about using actual 0 instead of NULL, maybe that's why your clang didn't warn?

To me, this patch is blah (sorry, I already don't like c++, much less using newer bloated standards of it.)

myQwil commented 8 months ago

I think there's an example that illustrates the problem pretty well:

#include <iostream>

struct B {};

struct A
{
    operator B*() {return 0;}
    operator bool() {return true;}
};

int main()
{
    A a;
    B* pb = 0;
    typedef void* null_ptr_t;
    null_ptr_t null = 0;

    std::cout << "(a == pb): " << (a == pb) << std::endl;
    std::cout << "(a == 0): " << (a == 0) << std::endl; // no warning
    std::cout << "(a == NULL): " << (a == NULL) << std::endl; // warns sometimes
    std::cout << "(a == null): " << (a == null) << std::endl;
}
(a == pb): 1
(a == 0): 0
(a == NULL): 0
(a == null): 1

My understanding is that it looks at 0 and NULL and thinks "These are more similar to booleans than pointers, so I'm going to convert a to a boolean."

At some point, we start getting into the realm of ambiguity and it's probably better if we're just really explicit about our intentions with the code.

@sezero I understand that some individuals may have reservations about adopting newer features and standards in programming languages, but the introduction of nullptr in C++11 was driven by the desire to improve code safety, clarity, and expressiveness.

Consistently using nullptr throughout a codebase can improve code readability and maintainability. It establishes a clear convention and avoids mixing different null representation styles, such as NULL and 0, which can lead to confusion and inconsistencies.