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

fix some syntax errors when compiling via msbuild 2019 compiler toolchain #763

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

Conversation

jstine35
Copy link

Notable change is replacing non-standard ssize_t with standard intmax_t (typically size_t is being depreciated in favor of intmax_t)

Everything else is pretty vanilla Win32 / msvc maintenance, very low risk of unwanted side effects.

@jstine35
Copy link
Author

jstine35 commented Nov 24, 2020

Update: Included more fixes, this time for linker warnings/errors also encountered when building DLL for Windows, via MSVC 2019.

Linker errors all appear to be legitimate type mismatches, though in the context of operation are fortunately harmless.

Especially interesting is the extern bool reference in C++ to a C-declared FastSaveStates (wherein bool is a macro for char or unsigned char or similar). This apparently works on llvm/gcc despite C++ name mangling which is typically present on such types. (C++ standard imposes no standard on name mangling, so it's possible to have one compiler mangle the name and another to happenstance into matching the C language's unmangled exported symbol).

The size of bool is not well defined in C++ and so I opted to use a well-defined datatype for C/C++ interop, in this case:

uint8_t FastSaveStates = false;

@pcercuei
Copy link
Contributor

Where did you read that ssize_t is deprecated? That sounds like complete BS to me.

Note that by changing ssize_t to intmax_t, you change the type from 32 to 64+ bits on 32-bit platforms. Not sure that's a smart idea.

As for your C/C++ bool compat, what about using the C99 _Bool type?

@jstine35
Copy link
Author

jstine35 commented May 10, 2021

ssize_t is still in POSIX but was rejected by both C99 and C++, and there appears to be broad consensus among those language standards folks to not accept it. There was more interest in converting size_t to signed - and I don't think that will ever happen either so that shows you where ssize_t sits on the realm of future integration into language standards.

The note about the change in size from 32 to 64 bit is fair criticism. This would be a problem in debug builds at least - compilers will optimize the c;lamp operation into the liveness of the inputs in any case where function inlining is supported, so performance generally should be unaffected.

More to your point though - it's not even assured that size_t is 32 bits on a 32-bit platform. In some cases it had been promoted to 64 bits in some edge-case 32-bit OSes that supported clever means of accessing large files and large RAM (>4GB). Later, size_t was largely kept 32-bit and a secondary 64-bit "offs_t" would be used - also not really standard - to avoid causing problems. But yea, ultimately, if we want to assure clamp is fast and efficient, we should actually micro-manage the type it uses.

or use sintptr_t .. which is part of all current language standards and almost always reflects the ideal optimized integer size of the platform (since a platform that has a slow intptr_t would be a really, really difficult platform to code anything that runs fast, hah). I'll circle back and fix that one.

_Bool doesn't exist on all C++ compilers since not all C++ compilers are C99 compliant. It is also, same as bool not strictly defined. It can be any size. Typically this is one byte, but who knows. Such types should be avoided for dynamically linked function definitions.

(honestly even using anything other than int is kind of risky. char, short, long, and long long are all vaguely defined and potentially compiler dependent. Technically int is too, but it's at least something that tends to follow a platform or OS defined ABI rather than a compiler whim)

@pcercuei
Copy link
Contributor

Ok, good to know about ssize_t. I guess intptr_t would be a better match than intmax_t, since you want the type to be "large enough", not "as large as possible".

As far as I know the implementation details of the C99 _Bool and the C++ bool are platform-dependent, not compiler-dependent, therefore you can always assume that they have the same size, and you can use them for interoperability. Same for all other major types.

@jstine35
Copy link
Author

I swapped in ptrdiff_t

I haven't found a suitable replacement for bool other than explicitly specifying uint8_t - Microsoft C++ v14.2 throws a hard error on the type mismatch of C++ bool to anything else (I cannot verify C99 _Bool since MSVC does not implement C99).

In any case, there is no such thing as a platform defined type in C/C++. Things are either language defined or undefined. Compilers can choose to follow platform guidelines in cases where the language leaves something undefined but that's about it. And as for bool there isn't even a platform guideline.

The BOOL type used by Windows APIs (shared libs) is 32-bits, while bool in all Windows C++ is a byte. So yes, even within the platform there's no defined standard for boolean.

@pcercuei
Copy link
Contributor

@jstine35 MSVC will happily accept the _Bool type.

 * Replace C bool macro with _Bool compiler type
 * C99 _Bool and C++ bool are considered matching types by the compiler
... and as a rule of thumb, always favor signed int unless there is specific reason to use unsigned (normally limited to situations where full 4GB unsigned range is required during comparison operation)
@jstine35
Copy link
Author

Ah, yes, it seems _Bool does work. I'm fairly used to MSVC rejecting most things from C99, I just gave up trying some years ago. (and MS said they had no plans to further support of C99, etc)

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.

2 participants