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

some CmdLineParser-related cleanups #7105

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

Conversation

firewave
Copy link
Collaborator

No description provided.

@firewave
Copy link
Collaborator Author

I filed https://trac.cppcheck.net/ticket/13431 about consolidating the XML CLI options.

@firewave firewave marked this pull request as ready for review January 3, 2025 10:03
@firewave
Copy link
Collaborator Author

firewave commented Jan 3, 2025

@@ -1005,6 +1003,7 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a

else if (std::strncmp(argv[i], "--output-format=", 16) == 0) {
const std::string format = argv[i] + 16;
// TODO: text and plist is missing
Copy link
Owner

Choose a reason for hiding this comment

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

well.. plist requires a filepath option so you can't just use --output-format=plist. you still have to provide --plist-output also and then this is redundant.

And I didn't see the point to write --output-format=text that seems redundant also.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well.. plist requires a filepath option so you can't just use --output-format=plist. you still have to provide --plist-output also and then this is redundant.

Makes sense.

And I didn't see the point to write --output-format=text that seems redundant also.

It would help to override some previously arguments which might be immutable.

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 still need some handling if plist is overriden since --plist-output sets two fields and --output-format only sets one of them. So the TODO is still valid.

Copy link
Owner

Choose a reason for hiding this comment

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

We still need some handling if plist is overriden since --plist-output sets two fields and --output-format only sets one of them. So the TODO is still valid.

ok

@@ -134,7 +134,7 @@ class CPPCHECKLIB WARN_UNUSED Settings {
/**
* Check code in the headers, this is on by default but can
* be turned off to save CPU */
bool checkHeaders = true;
bool checkHeaders = true; // TODO: CLI
Copy link
Owner

Choose a reason for hiding this comment

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

Well I did have a intention here. My idea was to only provide certain advanced options in the GUI. In an effort to keep the command line options simple.
It's a good thing if options are available on the command line.
It's a bad thing that the command line is "bloated".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well I did have a intention here. My idea was to only provide certain advanced options in the GUI. In an effort to keep the command line options simple.

Since this is a GUI project option it would be good to have an CLI option so you could override it since the project file might be immutable. Although CLI and GUI projects do not play well with each other at the moment - see https://trac.cppcheck.net/ticket/12918.

It's a good thing if options are available on the command line.
It's a bad thing that the command line is "bloated".

Well - that's kind of the point of a CLI - it should allow you to automate things all things you could do in the GUI. It also makes writing tests easier since you do not require to generate a project file.

Copy link
Owner

Choose a reason for hiding this comment

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

you can set the option in the gui project file though so you CAN use the cli and automate all things. It's a XML format file so not hard to write.

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 something like ffmpeg is an example of where I don't want to end up:
https://gist.github.com/tayvano/6e2d456a9897f55025e25035478a3a50

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you can set the option in the gui project file though so you CAN use the cli and automate all things. It's a XML format file so not hard to write.

The GUI project is inferior to the CLI. It is not documented. It only has a subset of the CLI options. The handling is different or just missing because that logic lives in the CmdLineParser or the GUI description.

For this to work properly it would require a rework. The easiest way would be to treat it more like a front-end than a GUI by generating CLI options which are then fed to the CmdLineParser. This would actually simplify a lot of things and probably get rid of a lot of duplicated (and possibly untested) code.

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 something like ffmpeg is an example of where I don't want to end up

Bad example. ffmpeg is a swissarmy knife.

Cppcheck can easily be used and doesn't require a configuration. Especially if you pass in an external project. But it has a lot of options and knobs (and we are still missing a lot - but we also have some which we probably do not need to have) and those should be able to be tuned without modifying the source.

"Easy to learn, hard to master".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also this option in particular might be leveraged to implement https://trac.cppcheck.net/ticket/13204.

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