Check Type::Native in isWindows() (f'up to #11917)
What does this fix? Why no tests?
See https://github.com/danmar/cppcheck/pull/5428
Platform is actually representing the compiler and not an operating system. So there are differences between running on Windows, using the Windows APIs and using Microsoft compiler extensions. So that might be a bandaid for only part of things.
So the platforms should actually be called something like msc or msvc or mingw or whatever and indicate if Microsoft extensions are supported (which is actually a compiler flag in Clang and GNU - would be kind of similar to -f{un}signed-char in that scope). The Windows API usage would be handled --library=windows. The handling of ANSI or Unicode functions I can't think of on the top of my head. Is that similar to architecture? Can we even handle big-endian/little-endian platforms with their different byte order?
This is basically best case scenario. I have several steps in that direction but as usual it's all too entangled and then we also have built-in platforms which make things more difficult.
I won't be able to get into this until tomorrow or even Wednesday.
After upgrading to head, I started getting lots of unknownMacro errors for _T(), because I was not passing an explicit platform. If native is actually supposed to work, then that Windows-specific stuff needs to run (although it might be better to have that in windows.cfg (but then you can't distinguish between ANSI and UNCODE anymore, although the former is pretty irrelevant by now)).
After upgrading to head, I started getting lots of
unknownMacroerrors for_T(), because I was not passing an explicit platform.
Okay, that was the information I was lacking. 👍
I am fine with having this change in for now, but let please me have a look first.
Okay, I had a look.
The functions check for the Windows platform to do Windows API adjustments but call it "Microsoft" which is semantically misleading (__try would be a Microsoft extension).
There's also some other ansi/unicode handling mapping in the code. This should actually be in the configuration (_T is already declared in wxwidgets.cfg).
Having ANSI/Unicode in the platform is quite unfortunately and completely misleading as it is not a global switch in the compiler but part of the Windows API/headers actually controlled by the UNICODE define - so this is a very granular thing. So what we are currently doing is quite wrong. I get why this was done as it was easy and convenient with ease of usage.
So we should do the following:
- move these symbols to the configuration and replace them with macros which consider
UNICODE - deprecate platforms
win32Aandwin32W(without a target version for now) and guide users to specify-DUNICODEand--library=windowsor to use--projectif they don't - specify
UNICODEifwin32Wis used - try to make some platform/API dependencies more generic (would be done in preparation already)
I will file tickets about this and check my countless platform-related branches/PRs for more things.
For now this hack should be fine as but we should start this during this development cycle as the migration will probably drag on.
FYI: The _T macro from wxwidgets is defined here: https://docs.wxwidgets.org/3.0/group__group__funcmacro__string.html
There's also some other ansi/unicode handling mapping in the code. This should actually be in the configuration (
_Tis already declared inwxwidgets.cfg).
Wrongly defined though (there is no check for UNICODE).
Wrongly defined though (there is no check for
UNICODE).
As I see it currently no code on our side is doing it correctly. So no harm done. 😉
See #4325, #4853, #4960 for some PR which try to explore on how to resolve parts of this.
Now 2.14 has been released, and the issue is still present (an explicit platform must be passed to handle e.g. _T() macros).
Yeah, I dropped the ball on this, sorry. Just too many things. I hope to take a look soon.