Skip to content

Conversation

@firewave
Copy link
Collaborator

@firewave firewave commented Sep 15, 2022

Extracted from #4325.

This is the first step in getting rid of hard-coded platforms and should have no effect at all beside making it consistent (and uncovering issues).

@firewave firewave marked this pull request as draft September 15, 2022 18:28
@firewave firewave changed the title set proper platform type for unix32-unsigned and unix64-unsigned set proper platform type for unix32-unsigned and unix64-unsigned / improved TestCmdlineParser Sep 17, 2022
@firewave firewave changed the title set proper platform type for unix32-unsigned and unix64-unsigned / improved TestCmdlineParser set proper platform type for unix32-unsigned and unix64-unsigned / copy platforms in CMake / improved TestCmdlineParser Sep 17, 2022
@firewave firewave changed the title set proper platform type for unix32-unsigned and unix64-unsigned / copy platforms in CMake / improved TestCmdlineParser fixed platforms lookup / set proper platform type for unix32-unsigned and unix64-unsigned / copy platforms in CMake Sep 17, 2022
@firewave firewave marked this pull request as ready for review September 17, 2022 17:33
bool success = false;
for (const std::string & f : filenames) {
if (verbose)
std::cout << "try to load platform file '" << f << "' ... ";
Copy link
Owner

Choose a reason for hiding this comment

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

We should not output any important information with std::cout directly. Is it just for debugging? It will not be shown properly in the GUI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's verbose information. I just checked other code and that uses the ErrorLogger::reportOut(). Will adjust.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Turns out that is not the function to use since it is not available at that point.

Also the rest of the code is not doing this consistently either. So I propose to apply these changes as is. With a later PR I will refactor out the explicit std::cout and std::cerr usage as a whole.

@firewave firewave marked this pull request as draft October 13, 2022 09:41
@firewave firewave marked this pull request as ready for review November 20, 2022 13:11
@firewave
Copy link
Collaborator Author

Can this be merged? It blocks further improvements.

@danmar danmar merged commit e557283 into danmar:main Jan 26, 2023
@firewave firewave deleted the platform-xyz branch January 26, 2023 21:11
@chrchr-github
Copy link
Collaborator

After this change. some tests in TestCmdlineParser fail on Windows using the supplied project files:

Testing Complete
Number of tests: 136
Number of todos: 16
Tests failed: 3

XXX\cppcheck\test\testcmdlineparser.cpp:1025(TestCmdlineParser::platformUnix32Unsigned): Assertion failed.
_____
XXX\cppcheck\test\testcmdlineparser.cpp:1043(TestCmdlineParser::platformUnix64Unsigned): Assertion failed.
_____
XXX\cppcheck\test\testcmdlineparser.cpp:1070(TestCmdlineParser::platformPlatformFile): Assertion failed.
Expected:
1

Actual:
0

_____

@firewave
Copy link
Collaborator Author

firewave commented Feb 1, 2023

I baffled as the CI is fine. Maybe the platform files were not properly copied on your system.

@chrchr-github
Copy link
Collaborator

Apparently, the test is executed in the test subdirectory when using the default setting for Working Directory = $(ProjectDir).
The tests pass when adding filenames.push_back("../platforms/" + filename + ".xml");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants