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.

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

added lowercase aliases win32a and win32w for platform types win32A and win32W - and use them internally #4853

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Mar 4, 2023

This allows us to simply load the given library from the filesystem instead of converting it. See #4325 for the PR to get rid of the hard-coded platform data.

@danmar
Copy link
Owner

danmar commented Mar 4, 2023

As far as I understand an alternative is to name the platform file "win32A.xml" and "win32W.xml" instead.

I do not know if users prefer win32A or win32a.

@firewave
Copy link
Collaborator Author

firewave commented Mar 4, 2023

All the external files have been lowercase so far and I would like to keep that. Otherwise it would open the door for allowing mixed casing and that would lead to different behavior based on the filesystem.

And if we just use lowercase files and convert all the input strings that would allow any casing to be found which also isn't nice.

These might also be referenced in other configurations where I am currently not sure how that is even used (seems we are lacking test coverage there).

We should have it consistent and simple. That would also require less tests to be written to assure it actually works in all cases.

@firewave firewave force-pushed the platform-d branch 3 times, most recently from 1479e9c to 77f1991 Compare April 8, 2023 11:22
@danmar
Copy link
Owner

danmar commented Apr 9, 2023

If we internally say win32A or win32a I don't care at all. I don't feel sure it's necessary to break the user interface. I mean this does not cause any users problems today as far as I know. And for us, I don't see that there is a big issue.

We could have a hardcoding in our code so "win32A" means "win32a" and "win32W" means "win32w". That is 2 strings to handle. Not very much. Then it will be backwards compatible.

.. do we really break the users scripts/plugins etc for very good reasons?

@firewave
Copy link
Collaborator Author

If we internally say win32A or win32a I don't care at all. I don't feel sure it's necessary to break the user interface. I mean this does not cause any users problems today as far as I know. And for us, I don't see that there is a big issue.

We could have a hardcoding in our code so "win32A" means "win32a" and "win32W" means "win32w". That is 2 strings to handle. Not very much. Then it will be backwards compatible.

.. do we really break the users scripts/plugins etc for very good reasons?

I want to get rid of special handling even if it just a line or two. Those things add up and they all need tests and handled in possible multiple cases.

The goal here is get rid of hard-coded stuff and make it all generic. I have several follow-up PRs on this which I will post soon. Maybe it makes more sense if you see where I want to head.

@firewave
Copy link
Collaborator Author

See #4960.

@danmar
Copy link
Owner

danmar commented Apr 30, 2023

Here is our list of "client and plugins" from our webpage:

Buildbot - integrated
CLion - Cppcheck plugin
Code::Blocks - integrated
CodeDX (software assurance tool) - integrated
CodeLite - integrated
CppDepend 5 - integrated
Eclipse - Cppcheclipse
gedit - gedit plugin
github - Codacy and SoftaCheck
Hudson - Cppcheck Plugin
Jenkins - Cppcheck Plugin
KDevelop - integrated since v5.1
Mercurial (Linux) - pre-commit hook - Check for new errors on commit (requires interactive terminal)
QtCreator - Qt Project Tool (qpt)
Tortoise SVN - Adding a pre-commit hook script
Visual Studio - Visual Studio plugin

So what is our plan to update these..

@firewave
Copy link
Collaborator Author

firewave commented Mar 5, 2024

I get the argument but we probably have broken things even worse recently.

I adjusted the code so it now states that it will be removed in an unspecified future version. So things will at least get cleaned up in some cases - mostly in out own usage.

@firewave firewave force-pushed the platform-d branch 2 times, most recently from a016bb6 to 18ab7d9 Compare March 5, 2024 16:30
@firewave firewave marked this pull request as ready for review March 5, 2024 16:30
lib/platform.cpp Outdated
else if (platformstr == "win32a")
set(Type::Win32A);
else if (platformstr == "win32W") {
std::cout << "Platform 'win32W' is deprecated and will be removed in a future version. Please use 'win32w' instead." << std::endl;
Copy link
Owner

@danmar danmar Mar 8, 2024

Choose a reason for hiding this comment

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

this message will not be handled well by some plugins. i.e. the vscode plugin I use will determine that cppcheck output is invalid and will not show any warnings..
I think we usually write something like "cppcheck: error: ..." when there are configuration problems.
however I fear that even "cppcheck: error: ..." would not be handled well by all plugins ideally we would report all the issues using proper ErrorMessage output... but I think writing proper ErrorMessage output can be done in a separate future PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If that would cause plugins to break we already did that quite a while ago with checkersReport (that actually broke the CLion plugin) and the "active checks" output as well as other deprecations way before that.

but I think writing proper ErrorMessage output can be done in a separate future PR.

Yes, so that everything results in a XML entry. I have that planned. After I am done with all my current tasks that will be the next to look into as it blocks further deprecations and changes like this.

Copy link
Owner

Choose a reason for hiding this comment

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

If that would cause plugins to break we already did that quite a while ago with checkersReport

it is reported as a normal information message nowadays.

Copy link
Owner

Choose a reason for hiding this comment

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

I think that we could at least always use the same format for bad command line arguments:

$ ./cppcheck --not-exist
cppcheck: error: unrecognized command line option: "--not-exist".

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 think that we could at least always use the same format for bad command line arguments:

That is (implicitly) part of the plan.

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 will introduce the lower-case aliases without a deprecation warning.

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 filed https://trac.cppcheck.net/ticket/13262 about deprecating the mixed case versions.

@firewave firewave changed the title deprecated platform types win32A and win32W in favor of win32a and win32w added lowercase aliases win32a and win32w for platform types win32A and win32W - and use them internally Oct 25, 2024
…32A` and `win32W` - and use them internally
@firewave
Copy link
Collaborator Author

It is possible these variants make no sense at all as this is not actually a platform. It depends on the UNICODE preprocessor. So win32W should actually be represented by --platform=win32 -DUNICODE. I have some old changes and/or observations in other PRs/branches I need to look up.

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.

2 participants