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

Add C++20 support for Deathmatch project #3745

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

TracerDS
Copy link
Contributor

No description provided.

@G-Moris
Copy link
Contributor

G-Moris commented Sep 23, 2024

Doubtful, because of linux

@TracerDS
Copy link
Contributor Author

Doubtful, because of linux

it compiled fine on linux, what are you on about?

@G-Moris
Copy link
Contributor

G-Moris commented Sep 23, 2024

Doubtful, because of linux

it compiled fine on linux, what are you on about?

It is possible that problems may arise in other linuxs, it may have already been solved, it is possible that this is so

@TracerDS
Copy link
Contributor Author

Doubtful, because of linux

it compiled fine on linux, what are you on about?

It is possible that problems may arise in other linuxs, it may have already been solved, it is possible that this is so

If it compiles fine on x86_64-gcc-10 and arm-gcc-10 then it should compile for other modern compilers

@TheNormalnij
Copy link
Member

If it compiles fine on x86_64-gcc-10 and arm-gcc-10 then it should compile for other modern compilers

It doesn't work like this

@TracerDS
Copy link
Contributor Author

TracerDS commented Sep 23, 2024

If it compiles fine on x86_64-gcc-10 and arm-gcc-10 then it should compile for other modern compilers

It doesn't work like this

I know, but if it matches the standard then it should work on most compilers that conform to the C++ standard.
There arent any major runtime changes (beside a fix). It should not have any implications

@TheNormalnij
Copy link
Member

I know, but if it matches the standard then it should work on most compilers that conform to the C++ standard.
There arent any major runtime changes (beside a fix). It should not have any implications

All compilers have different standard realization + specific bugs

@TracerDS
Copy link
Contributor Author

TracerDS commented Sep 23, 2024

I know, but if it matches the standard then it should work on most compilers that conform to the C++ standard.
There arent any major runtime changes (beside a fix). It should not have any implications

All compilers have different standard realization + specific bugs

As for this, none of the supported compilers (github actions) present a bug so we should be fine.
The only thing that could potentially be troublesome is the [[maybe_unused]] but even that is supported by most clang and gcc compilers.
Nothing to worry about.

I'd also assume it was thoroughly tested before rolling out C++23 version 😛

@TheNormalnij
Copy link
Member

As for this, none of the supported compilers (github actions) present a bug so we should be fine.
The only thing that could potentially be troublesome is the [[maybe_unused]] but even that is supported by most clang and gcc compilers.
Nothing to worry about.

You forgot about real-time bugs

@TracerDS
Copy link
Contributor Author

You forgot about real-time bugs

As I said before, the only real implication would be the change from __attribute__((unused)) to [[maybe_unused]].
Every other change is just an explicit SString call

@tederis
Copy link
Member

tederis commented Oct 7, 2024

This PR is highly desirable. At least because another one PR (#3287) depends on it. I think C++20 in this module will be fine and does not cause any problems. I already have an experience with C++20 in MTA and everything is going well.

P.S. In theory there can be ABI inconsistency if some of STL structures get returned from Deathmatch module. But these potential points of inconsistency are easy to detect in most cases. At least, it can be certain that MSVC is stable enough.

@TracerDS TracerDS requested a review from tederis October 8, 2024 10:37
@G-Moris
Copy link
Contributor

G-Moris commented Oct 19, 2024

Hello! Is there a problem with the support of other C++20 server modules?

@TracerDS
Copy link
Contributor Author

Hello! Is there a problem with the support of other C++20 server modules?

With some, probably. But I didnt check. Lets just get C++20 support for the deathmatch first

Comment on lines +1660 to +1661
const SString& strAccountName = parts.size() > 0 ? parts[0] : SString{};
const SString& strAction = parts.size() > 1 ? parts[1] : SString{};
Copy link
Contributor

Choose a reason for hiding this comment

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

Really should either use string_view or SString* here instead.

Copy link
Contributor Author

@TracerDS TracerDS Nov 28, 2024

Choose a reason for hiding this comment

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

i.e. const std::string_view/SString* strXYZ = ... : ""/nullptr?

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.

5 participants