Skip to content

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.

Sign up for GitHub

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

Merged
firewave merged 1 commit into from
Aug 8, 2023
Merged

CppcheckExecutor: use dedicated ErrorLogger for printing error messages XML #4985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions cli/cppcheckexecutor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,23 @@
#include <windows.h>
#endif

// TODO: do not directly write to stdout
class XMLErrorMessagesLogger : public ErrorLogger
{
void reportOut(const std::string & outmsg, Color /*c*/ = Color::Reset) override
{
std::cout << outmsg << std::endl;
}

void reportErr(const ErrorMessage &msg) override
{
reportOut(msg.toXML());
Copy link
Owner

@danmar danmar Aug 7, 2023

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

Copy link
Collaborator Author

@firewave firewave Aug 7, 2023 •
edited
Loading

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;
    }

Copy link
Owner

@danmar danmar Aug 7, 2023 •
edited
Loading

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.

Copy link
Collaborator Author

@firewave firewave Aug 7, 2023

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.

}

void reportProgress(const std::string & /*filename*/, const char /*stage*/[], const std::size_t /*value*/) override
Copy link
Collaborator Author

@firewave firewave Apr 19, 2023

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).

Copy link
Owner

@danmar danmar Aug 7, 2023

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...

Copy link
Owner

@danmar danmar Aug 7, 2023

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..

Copy link
Collaborator Author

@firewave firewave Aug 7, 2023

Choose a reason for hiding this comment

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

See #5269.

{}
};

// TODO: do not directly write to stdout

/*static*/ FILE* CppCheckExecutor::mExceptionOutput = stdout;

Expand Down Expand Up @@ -97,9 +112,9 @@ bool CppCheckExecutor::parseFromArgs(Settings &settings, int argc, const char* c
}

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;
}

Expand Down Expand Up @@ -409,11 +424,6 @@ void CppCheckExecutor::reportProgress(const std::string &filename, const char st

void CppCheckExecutor::reportErr(const ErrorMessage &msg)
{
if (mShowAllErrors) {
reportOut(msg.toXML());
return;
}

assert(mSettings != nullptr);

// Alert only about unique errors
Expand Down
5 changes: 0 additions & 5 deletions cli/cppcheckexecutor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,6 @@ class CppCheckExecutor : public ErrorLogger {
* Error output
*/
std::ofstream* mErrorOutput{};

/**
* Has --errorlist been given?
*/
bool mShowAllErrors{};
};

#endif // CPPCHECKEXECUTOR_H