-
Notifications
You must be signed in to change notification settings - Fork 1.5k
CppcheckExecutor: use dedicated ErrorLogger for printing error messages XML #4985
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
| reportOut(msg.toXML()); | ||
| } | ||
|
|
||
| void reportProgress(const std::string & /*filename*/, const char /*stage*/[], const std::size_t /*value*/) override |
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.
@danmar
I think we can remove reportProgress(). It is supposed to give progress messages at certain (albeit very few) stages which might be long-running. But actually getting such a message looks very unlikely and it doesn't really provide much value. It is not helpful to the user since there is nothing he can do about it. And if we get a report about a slow analysis this we simply profile it since we need the actual cause not just not some ballpark string.
I do see it being used as an cancellation callback but as that callback is hit very rarely that will probably not work as expected. That could be implemented via the Settings::terminated() check instead which is performed more often.
Sorry for having yet another off-topic discussion in a PR (we should really enable the discussions on the repo).
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.
The idea was that if analysis of a file takes minutes.. it will be helpful for the user to see that there is progress and that we haven't hanged. It was mostly for the GUI. Unfortunately it's not used in the GUI as far as I know it's not trivial to add it properly (technically it's easy but ui-wise not so easy). Technically we could add labels and progress bars for each thread in the mainwindow but it will waste precious space...
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.
In the command line a message is written every ~ 10 seconds. But the idea was to update the progress in the GUI more often maybe every seconds or so..
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.
See #5269.
|
This can be merged. I will add a new PR about the additional changes I suggested. |
|
|
||
| void reportErr(const ErrorMessage &msg) override | ||
| { | ||
| reportOut(msg.toXML()); |
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 have the feeling we want to write errors to std::cerr
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 is just for printing the XML and moves the previous code which did the same:
void CppCheckExecutor::reportErr(const ErrorMessage &msg)
{
if (mShowAllErrors) {
reportOut(msg.toXML());
return;
}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.
is this code only meant to be used if --errorlist is used? I assumed this errorlogger would be used to output warnings during normal analysis also.
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.
Yes.
if (parser.getShowErrorMessages()) {
mShowAllErrors = true;
XMLErrorMessagesLogger xmlLogger;
std::cout << ErrorMessage::getXMLHeader(settings.cppcheckCfgProductName);
CppCheck::getErrorMessages(*this);
CppCheck::getErrorMessages(xmlLogger);
std::cout << ErrorMessage::getXMLFooter() << std::endl;
}It should probably be moved somewhere else. I have a refactoring of the CmdLineParser which moves some things around which might help with this.
This PR is just about cleaning up the ErrorLogger implementation in CppcheckExecutor a bit.
a23a00e to
0204013
Compare
0204013 to
82c55b5
Compare
This starts to untangle the
ErrorLoggerimplementation inCppcheckExecutorwhich handles three different cases and makes things unnecessarily complicated.