Skip to content

Conversation

@RyeMutt
Copy link
Contributor

@RyeMutt RyeMutt commented Dec 4, 2025

Description

Modernize NULL with nullptr in newview for increased type safety and compatibility with modern c++ library expectations that compare to nullptr_t but have no compatibility for NULL.

Related Issues

Issue Link: #5078


Checklist

Please ensure the following before requesting review:

  • I have provided a clear title and detailed description for this pull request.
  • If useful, I have included media such as screenshots and video to show off my changes.
  • The PR is linked to a relevant issue with sufficient context.
  • I have tested the changes locally and verified they work as intended.
  • All new and existing tests pass.
  • Code follows the project's style guidelines.
  • Documentation has been updated if needed.
  • Any dependent changes have been merged and published in downstream modules
  • I have reviewed the contributing guidelines.

Additional Notes

@github-actions github-actions bot added the c/cpp label Dec 4, 2025
@akleshchev akleshchev requested a review from Geenz December 4, 2025 22:22
LPCWSTR unique_mutex_name = L"SecondLifeAppMutex";
HANDLE hMutex;
hMutex = CreateMutex(NULL, TRUE, unique_mutex_name);
hMutex = CreateMutex(nullptr, TRUE, unique_mutex_name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bulk searched & replaced? Windows API calls should IMHO keep using NULL.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about the reasoning for this. Not saying you are wrong but would like to learn the possible pitfalls with this.

Copy link
Contributor

@Ansariel Ansariel Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL equals to 0 and I don't think we should mess with the expected values of the Windows API. This and the other PR just look like a bulk search and replace change without taking OS-specific API implementations into account.

It is kinda weird this and the other PR use 3P compatibility as a reasoning for this change, but then completely disregards OS-specific API implementations. This line is just a placeholder for all changes using OS-specific API call and there a for sure a bunch more of these cases. In this particular case, you can even notice that TRUE hasn't changed to true for exactky that reason.

I don't dislike or dismiss the idea of this PR in general, but it seems like a bulk search and replace PR without taking individual cases into account.

Copy link
Contributor

@fmartian fmartian Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't argue with this. It probably is a bulk replace. What I wondered is, does nullptr not resolve to a the same actual 0x0000000000000000 value being passed on the stack? Maybe in certain compiler backends or whatever?

I can understand that the NULL was always a bit of a hack that was there because of the lack of a compiler native definition, but I would expect both to resolve ultimately to the same code unless there is some library specific optimization that a compiler "might" employ. For the WinAPI I would expect a compiler to stay far from trying to do any such silly things if it does not want to break anything in spectacular ways. But I'm learning here and just as with the STL being still quite an unfamiliar thing until now, I also am far from being even a C++ specialist.

Maybe there exist compilers that would bark on nullptr being passed to a standard C pointer of anything? Would seem weird but I'm not claiming it could not exist.

@akleshchev akleshchev requested a review from Copilot December 5, 2025 13:21
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants