Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use nullptr for all null pointer constants #79

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

myQwil
Copy link
Contributor

@myQwil myQwil commented Nov 9, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@sezero
Copy link
Contributor

sezero commented Nov 9, 2023

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
Copy link
Contributor Author

myQwil commented Nov 9, 2023

@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
Copy link
Contributor

sezero commented Nov 9, 2023

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
Copy link
Contributor Author

myQwil commented Nov 9, 2023

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.

Also add `-Wzero-as-null-pointer-constant` to cxx flags.
@sezero
Copy link
Contributor

sezero commented Nov 2, 2024

I think this can go in, yes?

@Wohlstand
Copy link
Collaborator

I want to merge this after nearest release. First that I should do is fixing the dual shared and static builds thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants