added lowercase aliases win32a and win32w for platform types win32A and win32W - and use them internally
#4853
Conversation
|
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. |
|
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
force-pushed
the
platform-d
branch
3 times, most recently
from
1479e9c
to
77f1991
Compare
|
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. |
|
See #4960. |
|
Here is our list of "client and plugins" from our webpage: Buildbot - integrated So what is our plan to update these.. |
|
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
force-pushed
the
platform-d
branch
2 times, most recently
from
a016bb6
to
18ab7d9
Compare
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
I think that we could at least always use the same format for bad command line arguments:
That is (implicitly) part of the plan.
There was a problem hiding this comment.
I will introduce the lower-case aliases without a deprecation warning.
There was a problem hiding this comment.
I filed https://trac.cppcheck.net/ticket/13262 about deprecating the mixed case versions.
firewave
changed the title
win32A and win32W in favor of win32a and win32wwin32a and win32w for platform types win32A and win32W - and use them internally
…32A` and `win32W` - and use them internally
|
It is possible these variants make no sense at all as this is not actually a platform. It depends on the |
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.