DescentDevelopers / Descent3

Descent 3 by Outrage Entertainment
GNU General Public License v3.0
2.74k stars 231 forks source link

More clean up #396

Closed GravisZro closed 1 month ago

GravisZro commented 1 month ago

Pull Request Type

Description

Clean up:

Checklist

winterheart commented 1 month ago

MAX_PATH is Windows compatibility macros. There no point renaming one C macro to other, this change is not giving any enhancement to the code.

GravisZro commented 1 month ago

That's why it's called clean up. The end result is there is only one macro defined to specify the maximum path length instead of three. That's enough of a benefit for me.

winterheart commented 1 month ago

I would retain MAX_PATH instead of MAX_PATH_ macros as MAX_PATH is part of Windows SDK:

#ifndef MAX_PATH
#define MAX_PATH 260
#endif
GravisZro commented 1 month ago

I would retain MAX_PATH instead of MAX_PATH_ macros as MAX_PATH is part of Windows SDK:

I actually had questions about MAX_PATH versus _MAX_PATH and it turns out that _MAX_PATH is specific to their implementation of libc while MAX_PATH part of WinSDK. https://learn.microsoft.com/en-us/cpp/c-runtime-library/path-field-limits

This is an important distinction because theoretically MAX_PATH could diverge from _MAX_PATH which means it could be an invalid value for the libc implementation. As such, the wise move is to use the version that is associated with the functions we use, which in this case are libc functions. That is why _MAX_PATH is the correct choice in this particular matter.

winterheart commented 1 month ago

There no MAX_PATH definition other than in Windows, so defining MAX_PATH on systems that is lacks of it (all other platforms except Windows) is most natural way.

GravisZro commented 1 month ago

@winterheart

Why define MAX_PATH at all? Why do we need a second definition for _MAX_PATH?

winterheart commented 1 month ago

Because is how it defined in Win SDK. On other platforms we need compatibility define.

GravisZro commented 1 month ago

On other platforms we need compatibility define.

No, you do not because MAX_PATH is no longer used in the D3 code base.

Lgt2x commented 1 month ago

Merging with @winterheart 's approval