CppcheckExecutor: use dedicated ErrorLogger for printing error messages XML#4985
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.
@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.
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.
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..
|
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.
I have the feeling we want to write errors to std::cerr
There was a problem hiding this comment.
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.
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.
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.