cppcheck icon indicating copy to clipboard operation
cppcheck copied to clipboard

Check Type::Native in isWindows() (f'up to #11917)

Open chrchr-github opened this issue 2 years ago • 12 comments

chrchr-github avatar Sep 18 '23 13:09 chrchr-github

What does this fix? Why no tests?

firewave avatar Sep 18 '23 14:09 firewave

See https://github.com/danmar/cppcheck/pull/5428

chrchr-github avatar Sep 18 '23 14:09 chrchr-github

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.

firewave avatar Sep 18 '23 14:09 firewave

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)).

chrchr-github avatar Sep 18 '23 15:09 chrchr-github

After upgrading to head, I started getting lots of unknownMacro errors 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.

firewave avatar Sep 18 '23 18:09 firewave

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 win32A and win32W (without a target version for now) and guide users to specify -DUNICODE and --library=windows or to use --project if they don't
  • specify UNICODE if win32W is 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.

firewave avatar Sep 18 '23 20:09 firewave

FYI: The _T macro from wxwidgets is defined here: https://docs.wxwidgets.org/3.0/group__group__funcmacro__string.html

orbitcowboy avatar Sep 18 '23 20:09 orbitcowboy

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).

Wrongly defined though (there is no check for UNICODE).

chrchr-github avatar Sep 18 '23 20:09 chrchr-github

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. 😉

firewave avatar Sep 18 '23 20:09 firewave

See #4325, #4853, #4960 for some PR which try to explore on how to resolve parts of this.

firewave avatar Sep 19 '23 16:09 firewave

Now 2.14 has been released, and the issue is still present (an explicit platform must be passed to handle e.g. _T() macros).

chrchr-github avatar Jan 03 '24 13:01 chrchr-github

Yeah, I dropped the ball on this, sorry. Just too many things. I hope to take a look soon.

firewave avatar Jan 04 '24 09:01 firewave