-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix #13825 (toomanyconfigs lacks file information) #7967
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
Conversation
|
Looks like we are lacking tests for this (and |
Yeah I was expecting some testfailures |
test/cli/other_test.py
Outdated
| f.write(f'#{dir} defined(X{i})\nx = {i};\n') | ||
| f.write('#endif\n') | ||
|
|
||
| args = ['--enable=information', '--template=daca2', str(test_file)] |
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.
Why are using the daca2 template?
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.
because default template writes 3 lines instead of 1.
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.
any 1-line template format will do here imho
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.
simple is usually used in the tests.
| // if maxConfigs has default value then report information message that configurations are skipped | ||
| if (mSettings.maxConfigs == 12 && mSettings.severity.isEnabled(Severity::information)) | ||
| tooManyConfigsError(Path::toNativeSeparators(file.spath()), configurations.size()); | ||
|
|
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 feels very strange.
Also the check for the default value should not be hard-coded - use a default Settings object to check against.
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 wrote a comment to describe it.
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.
Thanks. Seems to make sense but I have not thought too much about it - as I have not dug too much into #7424.
But I have to from a different angle anyways as part of preprocessor work I am doing. But merging this should be fine (possibly even helpful as there is less code to go through).
|



The file-less warning generated from cppcheckexecutor.cpp is removed. the --errorlist message should match the real message.