Skip to content

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

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.

Sign up for GitHub

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

Open
chrchr-github wants to merge 12 commits into
base: main
Choose a base branch
from
+26 −1
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f87a401
Check Type::Native in isWindows()
chrchr-github Sep 18, 2023
5d41d5d
Format
chrchr-github Sep 18, 2023
4e005c4
Add test
chrchr-github Sep 18, 2023
2d45605
Add another test
chrchr-github Sep 18, 2023
a37a580
Restrict
chrchr-github Sep 18, 2023
2f97117
Format
chrchr-github Sep 19, 2023
3afdd72
Merge branch 'main' into chr_iswindows
chrchr-github Jan 12, 2026
cf9921d
Update testplatform.cpp
chrchr-github Jan 12, 2026
f8390f1
Update testplatform.cpp
chrchr-github Jan 12, 2026
858169a
Update testsimplifytokens.cpp
chrchr-github Jan 12, 2026
7114a34
Update testsimplifytokens.cpp
chrchr-github Jan 12, 2026
60f7361
Update testsimplifytokens.cpp
chrchr-github Jan 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/platform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,11 @@ class CPPCHECKLIB Platform {
bool isWindows() const {
return type == Type::Win32A ||
type == Type::Win32W ||
type == Type::Win64;
type == Type::Win64
#ifdef _WIN32
|| type == Type::Native
#endif
;
}

const char *toString() const {
Expand Down
11 changes: 11 additions & 0 deletions test/testplatform.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class TestPlatform : public TestFixture {
TEST_CASE(valid_config_win32w);
TEST_CASE(valid_config_unix32);
TEST_CASE(valid_config_win64);
TEST_CASE(valid_config_native);
TEST_CASE(valid_config_file_1);
TEST_CASE(valid_config_file_2);
TEST_CASE(valid_config_file_3);
Expand Down Expand Up @@ -205,6 +206,16 @@ class TestPlatform : public TestFixture {
ASSERT_EQUALS(64, platform.long_double_bit);
}

void valid_config_native() const {
Copy link
Collaborator

@firewave firewave Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@chrchr-github chrchr-github Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@firewave firewave Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@chrchr-github chrchr-github Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Platform platform;
PLATFORM(platform, Platform::Type::Native);
#ifdef _WIN32
ASSERT(platform.isWindows());
#else
ASSERT(!platform.isWindows());
#endif
}

void valid_config_file_1() const {
// Valid platform configuration with all possible values specified.
// Similar to the avr8 platform file.
Expand Down
10 changes: 10 additions & 0 deletions test/testsimplifytokens.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class TestSimplifyTokens : public TestFixture {
mNewTemplate = true;
TEST_CASE(combine_strings);
TEST_CASE(combine_wstrings);
TEST_CASE(combine_wstrings_Windows);
TEST_CASE(combine_ustrings);
TEST_CASE(combine_Ustrings);
TEST_CASE(combine_u8strings);
Expand Down Expand Up @@ -250,6 +251,15 @@ class TestSimplifyTokens : public TestFixture {
ASSERT_EQUALS(expected, tokenizer.tokens()->stringifyList(nullptr, false));
}

void combine_wstrings_Windows() {
#if defined(_WIN32) && defined(UNICODE)
Copy link
Collaborator

@firewave firewave Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...😪😶😭

Copy link
Collaborator Author

@chrchr-github chrchr-github Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@firewave firewave Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const char code[] = "const auto* f() {\n"
" return _T(\"abc\") _T(\"def\") _T(\"ghi\");\n"
"}";
ASSERT_EQUALS("const auto * f ( ) { return L\"abcdefghi\" ; }", tok(code, dinit(TokOptions, $.type = Platform::Type::Native)));
#endif
}

void combine_ustrings() {
const char code[] = "abcd = u\"ab\" u\"cd\";";

Expand Down
Loading