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 more macro guarding for Windows #44

Open
dsprenkels opened this issue May 31, 2023 · 5 comments
Open

Add more macro guarding for Windows #44

dsprenkels opened this issue May 31, 2023 · 5 comments
Labels

Comments

@dsprenkels
Copy link
Owner

dsprenkels commented May 31, 2023

In #43, @raidenluikang requests that more macro guarding should be added on windows.

And add more macro for detection Windows, for ex., boost predef uses these macros:

_WIN32
_WIN64
__WIN32__
__TOS_WIN__
__WINDOWS__

@raidenluikang Can you elaborate why this is necessary? Is the current state broken in any way?

@dsprenkels
Copy link
Owner Author

Note: I think that my original code was based on https://stackoverflow.com/q/2989810/5207081

@dsprenkels
Copy link
Owner Author

Note 2: The current active documentation from Microsoft still specifies that

_WIN32 Defined as 1 when the compilation target is 32-bit ARM, 64-bit ARM, x86, or x64. Otherwise, undefined.

Reference: https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170

The current code conforms completely to the stack. Unless the current state of the code is in some way broken, I don't really see a reason to update it?

@raidenluikang
Copy link

raidenluikang commented Jun 3, 2023

To be honest, I myself have not yet figured out this issue.
But, I found this reference https://sourceforge.net/p/predef/wiki/OperatingSystems/


Identification | _WIN16 | Defined for 16-bit environments 1
-- | -- | --
Identification | _WIN32 | Defined for both 32-bit and 64-bit environments 1
Identification | _WIN64 | Defined for 64-bit environments 1
Identification | __WIN32__ | Defined by Borland C++
Identification | __TOS_WIN__ | Defined by xlC
Identification | __WINDOWS__ | Defined by Watcom C/C++

Identification	_WIN16	Defined for 16-bit environments [1](http://msdn.microsoft.com/en-us/library/ff540443.aspx)
Identification	_WIN32	Defined for both 32-bit and 64-bit environments [1](http://msdn.microsoft.com/en-us/library/ff540443.aspx)
Identification	_WIN64	Defined for 64-bit environments [1](http://msdn.microsoft.com/en-us/library/ff540443.aspx)
Identification	__WIN32__	Defined by Borland C++
Identification	__TOS_WIN__	Defined by xlC
Identification	__WINDOWS__	Defined by Watcom C/C++

Seems, __WIN32__ , __TOS_WIN__ and __WINDOWS__ compiler specific macros on windows. They maybe needed in real code, may be not. I don't able to test it. But, I know boost predef lib uses all of this macros for detection windows target.

Maybe other macros useful for embedded windows targets)

@dsprenkels
Copy link
Owner Author

Maybe other macros useful for embedded windows targets

That is an interesting point you bring up. I have not heard of any users (yet) that use this library on embedded Windows platforms. If so, please reach out. :)

The way I see it right now is, I would happily add more guarding if it fixes an issue. However, I want to be able to test that it does actually fix an issue. I would rather not copy-paste code from another library without knowing why it is there.

Is it okay if I close this issue? If, in the future, we find a configuration on which the current header is broken, we can add the extra header.

@raidenluikang
Copy link

There are another point.
This link
https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170 which _WIN32 macro defined all windows targets is true for MSVC compiler.

Could you test another compilers: for example some of them - mingw, Borland C++, Intel C++, Embarcadero C++ Builder, Clang, TinyC compiler, Portable C compiler ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants