dcleblanc / SafeInt

SafeInt is a class library for C++ that manages integer overflows.
MIT License
216 stars 37 forks source link

Do not overload wchar_t in environments where it fails to compile. #25

Closed jaykrell closed 3 years ago

jaykrell commented 3 years ago

This provides compatibility with environments for which wchar_t is an alias for unsigned short, which is the historical case on Visual C++, and still with /Zc:wchar_t-.

That is, historically: typedef unsigned short wchar_t;

void f(unsigned short) { } void f(wchar_t) { } // error

Making wchar_t a unique builtin type is conformant, allows the above, and changes the name mangling in preexisting code.

jaykrell commented 3 years ago

https://github.com/dcleblanc/SafeInt/issues/22

dcleblanc commented 3 years ago

That would be reverting a change I made in 2004 😊

So, having types aliased shouldn’t bother anything. This is why int_traits exists – I never know whether long is 32 or 64 bits. If you didn’t have a wchar_t to start with, and aliased it (typically to unsigned short), then it should all work. BTW, on Linux, wchar_t is 32-bit.

Is this:

_CONSTEXPR14 operator wchar_t() const SAFEINT_CPP_THROW

Not working with your environment? I would expect that if it were #defined to something else, that the code would still do the right thing.

From: Jay Krell notifications@github.com Sent: Monday, December 7, 2020 5:12 PM To: dcleblanc/SafeInt SafeInt@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: [dcleblanc/SafeInt] Make operator wchar_t conditional on #if SAFEINT_SUPPORT_WCHAR which defaults to 1. (#25)

This provides compatibility with environments for which wchar_t is an alias for unsigned short, which is the historical case on Visual C++, and still with /Zc:wchar_t-.

That is, historically: typedef unsigned short wchar_t;

void f(unsigned short) { } void f(wchar_t) { } // error

Making wchar_t a unique builtin type is conformant, allows the above, and changes the name mangling in preexisting code.


You can view, comment on, or merge this pull request online at:

https://github.com/dcleblanc/SafeInt/pull/25

Commit Summary

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/dcleblanc/SafeInt/pull/25, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHX6YL5R4JRF3ZGPZN4PXADSTV4MTANCNFSM4URH7MUQ.

jaykrell commented 3 years ago

int and long are always unique types, it doesn't matter if they have the same size and signendess.

You can always write:

void f(int) { }
void f(long) { }

Consider this code:

void f(int) { }
typedef int foo;
void f(foo) { } // error f(int) already defined

That is what you have, essentially.

jaykrell commented 3 years ago

For wchar_t to be 32bits on Unix, I know, doesn't affect this PR. :)

jaykrell commented 3 years ago

Oh, and this is why, btw, you must write your code (the traits) largely in terms int, long, long long. And not int32, int64. You will miss something -- only getting two types instead of three.

jaykrell commented 3 years ago

Try compiling with `cl.exe /Zc:wchar_t-. Which a lot of code does.

dcleblanc commented 3 years ago

Yes, I'd forgotten about that switch. Is that actually biting you in any of your environments? If so, then the right place to set this define would be in the USE_C_HEADERS section, since everything else in the last 15 years has a native type. And what I'd like to do is see if there's any way to auto-detect that this switch has been set, though that might be tricky.

jaykrell commented 3 years ago

I don't think this should be combined with USE_C_HEADERS. The switch remains. "Cartesian product/decomposition" is the process here. :) It can be detected, sort of. But it is tricky. I'll get back to you maybe. Old Visual C++, they are aliases. New Visual C++, they are sometimes. If they are not aliases, then there are one or two macros defined: _WCHAR_T_DEFINED and _NATIVE_WCHAR_T_DEFINED Also, independent of the switch, you can overload on the unique type: __wchar_t But I'm just quoting documentation, and some obvious things breaks with old compiler. Which is why I omitted autodetect. But there might be something a little better.

It is not just old environments that work this way. Windows builds this way by default for compatibility, using quite new headers, compilers, etc.

jaykrell commented 3 years ago

And yes this breaks our environment.

jaykrell commented 3 years ago

Ok , blech, I don't think this is useful.

crtdefs.h:

#ifndef _WCHAR_T_DEFINED
typedef unsigned short wchar_t;
#define _WCHAR_T_DEFINED
#endif  /* _WCHAR_T_DEFINED */

The implication is that the compiler defines _WCHAR_T_DEFINED to prevent anyone else from providing the typedef. But once any C runtime header is included, you cannot tell from whence it came.

If you want to assume a new compiler, then just write:

void f(unsigned short);
void f(__wchar_t);

but what is more appropriate is: void f(unsigned short);

ifndef _MSC_VER void f(wchar_t); // assume native type elif _MSC_VER > xxx void f(__wchar_t); // newer msvc hsa native else // older msvc, only provide unsigned short endif

The trick is finding xxx, which is a long time ago, granted.

There might be SNIFNAE trick

if MSC_VER enable_if<is_not_same_type<wchar_t, unsigned short>> endif

but once we are into std::enable_if..not sure we can use that here. Maybe can be simulated? Thus, I punted to the user. :)

jaykrell commented 3 years ago

Ok here is one solution I found:

#if !defined(_MSC_VER) || defined(_NATIVE_WCHAR_T_DEFINED)

provide overload for wchar_t

#endif

That should automate this.

dcleblanc commented 3 years ago

fixed in branch