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.

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

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 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
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 @@ -139,7 +139,11 @@ namespace cppcheck {
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 @@ -34,6 +34,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 @@ -181,6 +182,16 @@ class TestPlatform : public TestFixture {
ASSERT_EQUALS(64, platform.long_long_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

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

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

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

cppcheck::Platform platform;
PLATFORM(platform, cppcheck::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