-
-
Notifications
You must be signed in to change notification settings - Fork 428
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
С++20 for Client #3636
base: master
Are you sure you want to change the base?
С++20 for Client #3636
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want to introduce C++20 for Client but you change server code?
You also didnt tackle any issues regarding game_sa and multiplayer_sa
Client/core/CCore.cpp
Outdated
@@ -60,13 +60,13 @@ static HMODULE WINAPI SkipDirectPlay_LoadLibraryA(LPCSTR fileName) | |||
const fs::path inLaunchDir = fs::path{FromUTF8(GetLaunchPath())} / "enbseries" / "enbhelper.dll"; | |||
|
|||
if (fs::is_regular_file(inLaunchDir, ec)) | |||
return Win32LoadLibraryA(inLaunchDir.u8string().c_str()); | |||
return Win32LoadLibraryA(inLaunchDir.string().c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leave it as u8string for utf8 compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this conversation marked as resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below. In C++20 path.u8string may/will return std::basic_string<char8_t> which is incompatible with std::string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something you're not aware of: in the MSVC STL implementation of std::filesystem::path
the actual path is stored as a wide string (using string_type = std::wstring
) and the path::u8string
function uses _Convert_wide_to_narrow
to convert the wide string to a narrow variant with the UTF-8 codepage there, but the path::string()
function uses the active code page (ACP), which isn't going to be UTF-8 in most cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something you're not aware of: in the MSVC STL implementation of
std::filesystem::path
the actual path is stored as a wide string (using string_type = std::wstring
) and thepath::u8string
function uses_Convert_wide_to_narrow
to convert the wide string to a narrow variant with the UTF-8 codepage there, but thepath::string()
function uses the active code page (ACP), which isn't going to be UTF-8 in most cases.
Should I use ToACP for u8string? I understood correctly? For example: ToACP(inLaunchDir.u8string()).c_str();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are going to call Windows API functions like LoadLibraryA
with the *A
suffix, then you need the active code page encoding (ACP), which you get from path::string()
. For any other code that expects UTF-8, you have to take the wide string path::wstring()
and convert it with ToUTF8
(one of our functions - but please double-check that it actually converts from wide to narrow UTF-8), or you continue using path::u8string()
but cast the values from char8_t*
to char*
, for example.
Client/core/CCore.cpp
Outdated
|
||
// Try to load enbhelper.dll from the GTA install directory second. | ||
const fs::path inGTADir = g_gtaDirectory / "enbseries" / "enbhelper.dll"; | ||
|
||
if (fs::is_regular_file(inGTADir, ec)) | ||
return Win32LoadLibraryA(inGTADir.u8string().c_str()); | ||
return Win32LoadLibraryA(inGTADir.string().c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leave it as u8string for utf8 compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c_str in 'u8' returns char8_t, which is not compatible with LPCSTR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it changes in the C++20. Nevermind then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I close this and the like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment
Client/core/CCore.cpp
Outdated
@@ -60,13 +60,13 @@ static HMODULE WINAPI SkipDirectPlay_LoadLibraryA(LPCSTR fileName) | |||
const fs::path inLaunchDir = fs::path{FromUTF8(GetLaunchPath())} / "enbseries" / "enbhelper.dll"; | |||
|
|||
if (fs::is_regular_file(inLaunchDir, ec)) | |||
return Win32LoadLibraryA(inLaunchDir.u8string().c_str()); | |||
return Win32LoadLibraryA(inLaunchDir.string().c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this conversation marked as resolved?
@@ -84,12 +84,11 @@ bool CCommands::Execute(const char* szCommandLine) | |||
bool CCommands::Execute(const char* szCommand, const char* szParametersIn, bool bHandleRemotely, bool bIsScriptedBind) | |||
{ | |||
// Copy szParametersIn so the contents can be changed | |||
char* szParameters = NULL; | |||
std::unique_ptr<char[]> szParameters = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you not using std::string
?
Client/core/CCommands.cpp
Outdated
@@ -99,23 +98,27 @@ bool CCommands::Execute(const char* szCommand, const char* szParametersIn, bool | |||
if (szParameters) | |||
{ | |||
// His line starts with '/'? | |||
if (*szParameters == '/') | |||
if (szParameters[0] == '/') | |||
{ | |||
// Copy the characters after the slash to the 0 terminator to a seperate buffer | |||
char szBuffer[256]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you already refactor this code, use a std::array
here.
const SString strVersionSortKey = pServer->strVersionSortKey + pServer->strTieBreakSortKey; | ||
|
||
const SString strPlayers = pServer->nMaxPlayers == 0 ? "" : SString("%d / %d", pServer->nPlayers, pServer->nMaxPlayers); | ||
const char* szPlayers = pServer->nMaxPlayers == 0 ? "" : SString("%d / %d", pServer->nPlayers, pServer->nMaxPlayers).c_str(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You create a SString
on the right side, and take the pointer to the internal heap allocated string (though small string optimization might work here and you would get a stack pointer instead), but after this statement/line, the SString will go through the destructor and your szPlayers
pointer will point to freed memory.
const SString strPlayersSortKey = SString("%04d-", pServer->nMaxPlayers ? pServer->nPlayers + 1 : 0) + pServer->strTieBreakSortKey; | ||
|
||
const SString strPing = pServer->nPing == 9999 ? "" : SString("%d", pServer->nPing); | ||
const char* szPing = pServer->nPing == 9999 ? "" : SString("%d", pServer->nPing).c_str(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same problem with the SString
. See other review comment.
Client/game_sa/CWeaponSA.cpp
Outdated
const CVector* pvecOrigin = &vecOrigin; | ||
const CVector2D* pvecDirection = &vecDirection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just lazy. Refactor the code to work with the references.
@@ -107,7 +107,7 @@ namespace SharedUtil | |||
{ | |||
#ifdef UTF8_FILE_HOOKS_PERSONALITY_Core | |||
static SString gtaDirCP = ToACP(g_gtaDirectory); | |||
static SString gtaDirUTF8 = g_gtaDirectory.u8string(); | |||
static SString gtaDirUTF8 = g_gtaDirectory.string(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::filesystem::path::string
does not return a narrow string with UTF-8 encoding.
@Dutchman101 Hello! Sorry for the trouble and for the fact that this is not on the topic of PR, can you please unblock me in the |
Maybe I'll try to solve it |
@Dutchman101 Hello |
this is not the place for that |
Other than that, I have nowhere to tell, where else should I write? |
Discord, forum. Anywhere but here |
I can't in the discord, on the forum too, what else? |
С++20 for Client, my test version