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

chrchr-github · 2023-09-18T13:19:53Z

No description provided.

firewave · 2023-09-18T14:27:17Z

What does this fix? Why no tests?

chrchr-github · 2023-09-18T14:37:07Z

firewave · 2023-09-18T14:54:13Z

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.

chrchr-github · 2023-09-18T15:14:32Z

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

firewave · 2023-09-18T18:22:34Z

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 · 2023-09-18T20:31:49Z

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.

orbitcowboy · 2023-09-18T20:37:20Z

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

firewave · 2023-09-18T20:37:55Z

test/testplatform.cpp

@@ -181,6 +182,16 @@ class TestPlatform : public TestFixture {
        ASSERT_EQUALS(64, platform.long_long_bit);
    }

+    void valid_config_native() const {


Could you also please add a test for the actual issue you encountered? That would make it much clearer what is intended here.

We should first decide whether _T() etc. are tied to a platform or a library. Then we can test if it is properly replaced. Btw, there are more calls to isWindows() which are also affected with native.

It is neither. It is tied to a user(!) defined macro.

It needs to be put into a library though as it comes from the API/headers and not the compiler.

We need to review all code but for now it should be fine to make the proposed change. That's why we have a development cycle so we are allowed to break things temporarily. And it is also why I did it immediately at the beginning of it so we have the time we need. For now I would just like to see a test for the actual problem you encountered.

I just noticed that we even have platform-dependent types in windows.cfg (e.g. TCHAR).

chrchr-github · 2023-09-18T20:41:30Z

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

firewave · 2023-09-18T20:43:09Z

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 · 2023-09-18T22:18:53Z

test/testsimplifytokens.cpp

@@ -270,6 +271,16 @@ class TestSimplifyTokens : public TestFixture {
        ASSERT_EQUALS(expected, tokenizer.tokens()->stringifyList(nullptr, false));
    }

+    void combine_wstrings_Windows() {
+#if defined(_WIN32) && defined(UNICODE)


That check doesn't seem to make sense. It should default to ANSI if just isWindows() is true. So we should test native on Windows and not Windows (with _WIN32 check) and also test the win32A and win32W platforms.

And I just realized that we only have win64. I guess that is used more than the 32-bit so there is no way to actually configure the Unicode versions for 64-bit Windows scans and the two win32 platform make even less sense and we could just deprecate those without doing any other changes whatsoever. This is such a mess...😪😶😭

Yes it is.
So will we be dropping support for anything but 64bit + UNICODE?
It's also kind of silly that we accept two unknown macros in a row (return _T(...) _T(...);), but throw an error for the third.

So will we be dropping support for anything but 64bit + UNICODE?

At least I will try to deprecate certain things and make them more generic. But I have already been doing that in a very iterative way for quite a while now. There' just two many things which are entangled and you need to wrap your head around.

The built-in platforms are a major pain as they spill into the GUI which makes things even more complicated as they are.

firewave · 2023-09-19T16:11:19Z

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

chrchr-github · 2024-01-03T13:04:32Z

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

firewave · 2024-01-04T09:10:07Z

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

chrchr-github added 2 commits September 18, 2023 15:19

Check Type::Native in isWindows()

f87a401

Format

5d41d5d

Add test

4e005c4

chrchr-github changed the title ~~Check Type::Native in isWindows()~~ Check Type::Native in isWindows() (f'up to #11917) Sep 18, 2023

firewave reviewed Sep 18, 2023

View reviewed changes

chrchr-github added 2 commits September 19, 2023 00:01

Add another test

2d45605

Restrict

a37a580

firewave reviewed Sep 18, 2023

View reviewed changes

Format

2f97117

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chrchr-github commented Sep 18, 2023

firewave commented Sep 18, 2023

chrchr-github commented Sep 18, 2023

firewave commented Sep 18, 2023

chrchr-github commented Sep 18, 2023 •

edited

Loading

firewave commented Sep 18, 2023

firewave commented Sep 18, 2023 •

edited

Loading

orbitcowboy commented Sep 18, 2023

firewave Sep 18, 2023 •

edited

Loading

chrchr-github Sep 18, 2023

firewave Sep 18, 2023

chrchr-github Sep 18, 2023

chrchr-github commented Sep 18, 2023

firewave commented Sep 18, 2023

firewave Sep 18, 2023

chrchr-github Sep 19, 2023

firewave Sep 19, 2023

firewave commented Sep 19, 2023

chrchr-github commented Jan 3, 2024

firewave commented Jan 4, 2024

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

Are you sure you want to change the base?

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

Conversation

chrchr-github commented Sep 18, 2023

firewave commented Sep 18, 2023

chrchr-github commented Sep 18, 2023

firewave commented Sep 18, 2023

chrchr-github commented Sep 18, 2023 • edited Loading

firewave commented Sep 18, 2023

firewave commented Sep 18, 2023 • edited Loading

orbitcowboy commented Sep 18, 2023

firewave Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

chrchr-github Sep 18, 2023

Choose a reason for hiding this comment

firewave Sep 18, 2023

Choose a reason for hiding this comment

chrchr-github Sep 18, 2023

Choose a reason for hiding this comment

chrchr-github commented Sep 18, 2023

firewave commented Sep 18, 2023

firewave Sep 18, 2023

Choose a reason for hiding this comment

chrchr-github Sep 19, 2023

Choose a reason for hiding this comment

firewave Sep 19, 2023

Choose a reason for hiding this comment

firewave commented Sep 19, 2023

chrchr-github commented Jan 3, 2024

firewave commented Jan 4, 2024

chrchr-github commented Sep 18, 2023 •

edited

Loading

firewave commented Sep 18, 2023 •

edited

Loading

firewave Sep 18, 2023 •

edited

Loading