-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix #11824: Option --max-configs has no effect if -D is used #7424
base: main
Are you sure you want to change the base?
Conversation
This needs a very thorough review as well as tests and documentation (in general and documenting this behavior change). As mentioned in the ticket we might need to fix other things first. We might also need to print some messages/error if certain configurations are incompatible (I have not looked at it at all yet). |
Basically I agree with the idea. If You can add an example that clarify this in the manual also: |
I would also like to see some regression tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember why we have added Settings::force
and Settings::checkAllConfigurations
. Isn't it enough with just Settings::maxConfigs
?
Imho testcmdlineparser.cpp should test that when --max-config=2 is used then Settings::maxConfigs will be 2 no matter if -D has been used or not.
Timer::run("Preprocessor::getConfigs", mSettings.showtime, &s_timerResults, [&]() { | ||
configurations = preprocessor.getConfigs(tokens1); | ||
}); | ||
Timer::run("Preprocessor::getConfigs", mSettings.showtime, &s_timerResults, [&]() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code will always execute Preprocessor::getConfigs
even if max configs is 1.
I think that has been grown historically. It is also possible that part of this is cleaned up with #7293. The duplicated and differing logic in the CLI and GUI code make things a bit awkward. |
yes.. if we can unify logic that would make me happy.. :-) |
Working on it... |
I have checked the comments on the ticket of the bug. And I simply summarized my logic of checking the configs as below.