Skip to content

Commit 1f2b491

Browse files
firewavefirewave
authored
extracted single job execution into SingleExecutor / improved testing / do not unconditionally apply colors to output (cppcheck-opensource#4882)
* added `Settings::useSingleJob()` and use it instead of checking `jobs` or `jointSuppressionReport` * extracted single job execution into `SingleExecutor` * moved `reportStatus()` from `CppCheckExecutor` to Èxecutor * TestSingleExecutor: improved tests * added testing of markup extension handling in executors * cleaned up includes based on `include-what-you-use` * testsingleexecutor.cpp: suppress `performance-unnecessary-value-param` clang-tidy warnings * ProcessExecutor: send color via pipe instead of applying it beforehand * do not unconditionally apply colors to output / disable all colors in tests / adjusted tests for changed output behavior * fixed precision loss in `Executor::reportStatus()` * fixed `naming-varname` selfcheck warnings
1 parent ba16847 commit 1f2b491

25 files changed

Lines changed: 551 additions & 119 deletions

‎Makefile‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,7 @@ CLIOBJ = cli/cmdlineparser.o \
261261
cli/filelister.o \
262262
cli/main.o \
263263
cli/processexecutor.o \
264+
cli/singleexecutor.o \
264265
cli/stacktrace.o \
265266
cli/threadexecutor.o
266267

@@ -310,6 +311,7 @@ TESTOBJ = test/fixture.o \
310311
test/testsimplifytokens.o \
311312
test/testsimplifytypedef.o \
312313
test/testsimplifyusing.o \
314+
test/testsingleexecutor.o \
313315
test/testsizeof.o \
314316
test/teststl.o \
315317
test/teststring.o \
@@ -342,7 +344,7 @@ cppcheck: $(LIBOBJ) $(CLIOBJ) $(EXTOBJ)
342344

343345
all: cppcheck testrunner
344346

345-
testrunner: $(TESTOBJ) $(LIBOBJ) $(EXTOBJ) cli/executor.o cli/processexecutor.o cli/threadexecutor.o cli/cmdlineparser.o cli/cppcheckexecutor.o cli/cppcheckexecutorseh.o cli/cppcheckexecutorsig.o cli/stacktrace.o cli/filelister.o
347+
testrunner: $(TESTOBJ) $(LIBOBJ) $(EXTOBJ) cli/executor.o cli/processexecutor.o cli/singleexecutor.o cli/threadexecutor.o cli/cmdlineparser.o cli/cppcheckexecutor.o cli/cppcheckexecutorseh.o cli/cppcheckexecutorsig.o cli/stacktrace.o cli/filelister.o
346348
$(CXX) $(CPPFLAGS) $(CXXFLAGS) -o $@ $^ $(LIBS) $(LDFLAGS) $(RDYNAMIC)
347349

348350
test: all
@@ -634,7 +636,7 @@ $(libcppdir)/vfvalue.o: lib/vfvalue.cpp lib/config.h lib/errortypes.h lib/mathli
634636
cli/cmdlineparser.o: cli/cmdlineparser.cpp cli/cmdlineparser.h cli/cppcheckexecutor.h cli/filelister.h externals/tinyxml2/tinyxml2.h lib/check.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/timer.h lib/utils.h
635637
$(CXX) ${INCLUDE_FOR_CLI} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ cli/cmdlineparser.cpp
636638

637-
cli/cppcheckexecutor.o: cli/cppcheckexecutor.cpp cli/cmdlineparser.h cli/cppcheckexecutor.h cli/cppcheckexecutorseh.h cli/cppcheckexecutorsig.h cli/executor.h cli/filelister.h cli/processexecutor.h cli/threadexecutor.h lib/analyzerinfo.h lib/check.h lib/checkunusedfunctions.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/path.h lib/pathmatch.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h
639+
cli/cppcheckexecutor.o: cli/cppcheckexecutor.cpp cli/cmdlineparser.h cli/cppcheckexecutor.h cli/cppcheckexecutorseh.h cli/cppcheckexecutorsig.h cli/executor.h cli/filelister.h cli/processexecutor.h cli/singleexecutor.h cli/threadexecutor.h lib/analyzerinfo.h lib/check.h lib/checkunusedfunctions.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/path.h lib/pathmatch.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h
638640
$(CXX) ${INCLUDE_FOR_CLI} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ cli/cppcheckexecutor.cpp
639641

640642
cli/cppcheckexecutorseh.o: cli/cppcheckexecutorseh.cpp cli/cppcheckexecutor.h cli/cppcheckexecutorseh.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/suppressions.h lib/utils.h
@@ -655,6 +657,9 @@ cli/main.o: cli/main.cpp cli/cppcheckexecutor.h lib/color.h lib/config.h lib/err
655657
cli/processexecutor.o: cli/processexecutor.cpp cli/cppcheckexecutor.h cli/executor.h cli/processexecutor.h lib/analyzerinfo.h lib/check.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h
656658
$(CXX) ${INCLUDE_FOR_CLI} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ cli/processexecutor.cpp
657659

660+
cli/singleexecutor.o: cli/singleexecutor.cpp cli/executor.h cli/singleexecutor.h lib/analyzerinfo.h lib/check.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h
661+
$(CXX) ${INCLUDE_FOR_CLI} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ cli/singleexecutor.cpp
662+
658663
cli/stacktrace.o: cli/stacktrace.cpp cli/stacktrace.h lib/config.h lib/utils.h
659664
$(CXX) ${INCLUDE_FOR_CLI} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ cli/stacktrace.cpp
660665

@@ -799,6 +804,9 @@ test/testsimplifytypedef.o: test/testsimplifytypedef.cpp externals/simplecpp/sim
799804
test/testsimplifyusing.o: test/testsimplifyusing.cpp lib/check.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h test/fixture.h
800805
$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testsimplifyusing.cpp
801806

807+
test/testsingleexecutor.o: test/testsingleexecutor.cpp cli/executor.h cli/singleexecutor.h lib/analyzerinfo.h lib/check.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/templatesimplifier.h lib/timer.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h test/fixture.h test/helpers.h test/redirect.h
808+
$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testsingleexecutor.cpp
809+
802810
test/testsizeof.o: test/testsizeof.cpp externals/simplecpp/simplecpp.h lib/check.h lib/checksizeof.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h test/fixture.h
803811
$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testsizeof.cpp
804812

‎cli/cli.vcxproj‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,7 @@
435435
<ClInclude Include="executor.h" />
436436
<ClInclude Include="filelister.h" />
437437
<ClInclude Include="processexecutor.h" />
438+
<ClInclude Include="singleexecutor.h" />
438439
<ClInclude Include="stacktrace.h" />
439440
<ClInclude Include="threadexecutor.h" />
440441
</ItemGroup>
@@ -461,6 +462,7 @@
461462
<ClCompile Include="filelister.cpp" />
462463
<ClCompile Include="main.cpp" />
463464
<ClCompile Include="processexecutor.cpp" />
465+
<ClCompile Include="singleexecutor.cpp" />
464466
<ClCompile Include="stacktrace.cpp" />
465467
<ClCompile Include="threadexecutor.cpp" />
466468
</ItemGroup>

‎cli/cppcheckexecutor.cpp‎

Lines changed: 10 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,10 @@
3030
#include "path.h"
3131
#include "pathmatch.h"
3232
#include "settings.h"
33+
#include "singleexecutor.h"
3334
#include "suppressions.h"
3435
#include "utils.h"
36+
3537
#include "checkunusedfunctions.h"
3638

3739
#if defined(THREADING_MODEL_THREAD)
@@ -51,7 +53,6 @@
5153
#include <sstream> // IWYU pragma: keep
5254
#include <utility>
5355
#include <vector>
54-
#include <numeric>
5556

5657
#ifdef USE_UNIX_SIGNAL_HANDLING
5758
#include "cppcheckexecutorsig.h"
@@ -242,7 +243,7 @@ bool CppCheckExecutor::reportSuppressions(const Settings &settings, bool unusedF
242243
return false;
243244

244245
bool err = false;
245-
if (settings.jointSuppressionReport) {
246+
if (settings.useSingleJob()) {
246247
for (std::map<std::string, std::size_t>::const_iterator i = files.cbegin(); i != files.cend(); ++i) {
247248
err |= errorLogger.reportUnmatchedSuppressions(
248249
settings.nomsg.getUnmatchedLocalSuppressions(i->first, unusedFunctionCheckEnabled));
@@ -310,54 +311,10 @@ int CppCheckExecutor::check_internal(CppCheck& cppcheck)
310311
}
311312

312313
unsigned int returnValue = 0;
313-
if (settings.jobs == 1) {
314+
if (settings.useSingleJob()) {
314315
// Single process
315-
settings.jointSuppressionReport = true;
316-
317-
const std::size_t totalfilesize = std::accumulate(mFiles.cbegin(), mFiles.cend(), std::size_t(0), [](std::size_t v, const std::pair<std::string, std::size_t>& f) {
318-
return v + f.second;
319-
});
320-
321-
std::size_t processedsize = 0;
322-
unsigned int c = 0;
323-
if (settings.project.fileSettings.empty()) {
324-
for (std::map<std::string, std::size_t>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
325-
if (!settings.library.markupFile(i->first)
326-
|| !settings.library.processMarkupAfterCode(i->first)) {
327-
returnValue += cppcheck.check(i->first);
328-
processedsize += i->second;
329-
if (!settings.quiet)
330-
reportStatus(c + 1, mFiles.size(), processedsize, totalfilesize);
331-
c++;
332-
}
333-
}
334-
} else {
335-
// filesettings
336-
// check all files of the project
337-
for (const ImportProject::FileSettings &fs : settings.project.fileSettings) {
338-
returnValue += cppcheck.check(fs);
339-
++c;
340-
if (!settings.quiet)
341-
reportStatus(c, settings.project.fileSettings.size(), c, settings.project.fileSettings.size());
342-
if (settings.clangTidy)
343-
cppcheck.analyseClangTidy(fs);
344-
}
345-
}
346-
347-
// TODO: not performed when multiple jobs are being used
348-
// second loop to parse all markup files which may not work until all
349-
// c/cpp files have been parsed and checked
350-
for (std::map<std::string, std::size_t>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
351-
if (settings.library.markupFile(i->first) && settings.library.processMarkupAfterCode(i->first)) {
352-
returnValue += cppcheck.check(i->first);
353-
processedsize += i->second;
354-
if (!settings.quiet)
355-
reportStatus(c + 1, mFiles.size(), processedsize, totalfilesize);
356-
c++;
357-
}
358-
}
359-
if (cppcheck.analyseWholeProgram())
360-
returnValue++;
316+
SingleExecutor executor(cppcheck, mFiles, settings, *this);
317+
returnValue = executor.check();
361318
} else {
362319
#if defined(THREADING_MODEL_THREAD)
363320
ThreadExecutor executor(mFiles, settings, *this);
@@ -423,8 +380,10 @@ void CppCheckExecutor::reportErr(const std::string &errmsg)
423380

424381
void CppCheckExecutor::reportOut(const std::string &outmsg, Color c)
425382
{
426-
// TODO: do not unconditionally apply colors
427-
std::cout << c << ansiToOEM(outmsg, true) << Color::Reset << std::endl;
383+
if (c == Color::Reset)
384+
std::cout << ansiToOEM(outmsg, true) << std::endl;
385+
else
386+
std::cout << toString(c) << ansiToOEM(outmsg, true) << toString(Color::Reset) << std::endl;
428387
}
429388

430389
void CppCheckExecutor::reportProgress(const std::string &filename, const char stage[], const std::size_t value)
@@ -450,19 +409,6 @@ void CppCheckExecutor::reportProgress(const std::string &filename, const char st
450409
}
451410
}
452411

453-
void CppCheckExecutor::reportStatus(std::size_t fileindex, std::size_t filecount, std::size_t sizedone, std::size_t sizetotal)
454-
{
455-
if (filecount > 1) {
456-
std::ostringstream oss;
457-
const long percentDone = (sizetotal > 0) ? static_cast<long>(static_cast<long double>(sizedone) / sizetotal * 100) : 0;
458-
oss << fileindex << '/' << filecount
459-
<< " files checked " << percentDone
460-
<< "% done";
461-
// TODO: do not unconditionally print in color
462-
std::cout << Color::FgBlue << oss.str() << Color::Reset << std::endl;
463-
}
464-
}
465-
466412
void CppCheckExecutor::reportErr(const ErrorMessage &msg)
467413
{
468414
if (mShowAllErrors) {

‎cli/cppcheckexecutor.h‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -81,16 +81,6 @@ class CppCheckExecutor : public ErrorLogger {
8181

8282
void reportProgress(const std::string &filename, const char stage[], const std::size_t value) override;
8383

84-
/**
85-
* Information about how many files have been checked
86-
*
87-
* @param fileindex This many files have been checked.
88-
* @param filecount This many files there are in total.
89-
* @param sizedone The sum of sizes of the files checked.
90-
* @param sizetotal The total sizes of the files.
91-
*/
92-
static void reportStatus(std::size_t fileindex, std::size_t filecount, std::size_t sizedone, std::size_t sizetotal);
93-
9484
/**
9585
* @param exceptionOutput Output file
9686
*/

‎cli/executor.cpp‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,14 @@
1818

1919
#include "executor.h"
2020

21+
#include "color.h"
2122
#include "errorlogger.h"
2223
#include "settings.h"
2324
#include "suppressions.h"
2425

2526
#include <algorithm>
27+
#include <iostream>
28+
#include <sstream> // IWYU pragma: keep
2629
#include <utility>
2730

2831
Executor::Executor(const std::map<std::string, std::size_t> &files, Settings &settings, ErrorLogger &errorLogger)
@@ -47,3 +50,15 @@ bool Executor::hasToLog(const ErrorMessage &msg)
4750
return false;
4851
}
4952

53+
void Executor::reportStatus(std::size_t fileindex, std::size_t filecount, std::size_t sizedone, std::size_t sizetotal)
54+
{
55+
if (filecount > 1) {
56+
std::ostringstream oss;
57+
const unsigned long percentDone = (sizetotal > 0) ? (100 * sizedone) / sizetotal : 0;
58+
oss << fileindex << '/' << filecount
59+
<< " files checked " << percentDone
60+
<< "% done";
61+
mErrorLogger.reportOut(oss.str(), Color::FgBlue);
62+
}
63+
}
64+

‎cli/executor.h‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,16 @@ class Executor {
4646

4747
virtual unsigned int check() = 0;
4848

49+
/**
50+
* Information about how many files have been checked
51+
*
52+
* @param fileindex This many files have been checked.
53+
* @param filecount This many files there are in total.
54+
* @param sizedone The sum of sizes of the files checked.
55+
* @param sizetotal The total sizes of the files.
56+
*/
57+
void reportStatus(std::size_t fileindex, std::size_t filecount, std::size_t sizedone, std::size_t sizetotal);
58+
4959
protected:
5060
/**
5161
* @brief Check if message is being suppressed and unique.

‎cli/processexecutor.cpp‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,7 @@ class PipeWriter : public ErrorLogger {
7373
explicit PipeWriter(int pipe) : mWpipe(pipe) {}
7474

7575
void reportOut(const std::string &outmsg, Color c) override {
76-
// TODO: do not unconditionally apply colors
77-
writeToPipe(REPORT_OUT, ::toString(c) + outmsg + ::toString(Color::Reset));
76+
writeToPipe(REPORT_OUT, static_cast<char>(c) + outmsg);
7877
}
7978

8079
void reportErr(const ErrorMessage &msg) override {
@@ -178,7 +177,9 @@ bool ProcessExecutor::handleRead(int rpipe, unsigned int &result, const std::str
178177

179178
bool res = true;
180179
if (type == PipeWriter::REPORT_OUT) {
181-
mErrorLogger.reportOut(buf);
180+
// the first charcater is the color
181+
const Color c = static_cast<Color>(buf[0]);
182+
mErrorLogger.reportOut(buf + 1, c);
182183
} else if (type == PipeWriter::REPORT_ERROR) {
183184
ErrorMessage msg;
184185
try {
@@ -328,7 +329,7 @@ unsigned int ProcessExecutor::check()
328329
fileCount++;
329330
processedsize += size;
330331
if (!mSettings.quiet)
331-
CppCheckExecutor::reportStatus(fileCount, mFiles.size() + mSettings.project.fileSettings.size(), processedsize, totalfilesize);
332+
Executor::reportStatus(fileCount, mFiles.size() + mSettings.project.fileSettings.size(), processedsize, totalfilesize);
332333

333334
close(*rp);
334335
rp = rpipes.erase(rp);

‎cli/singleexecutor.cpp‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
/*
2+
* Cppcheck - A tool for static C/C++ code analysis
3+
* Copyright (C) 2007-2023 Cppcheck team.
4+
*
5+
* This program is free software: you can redistribute it and/or modify
6+
* it under the terms of the GNU General Public License as published by
7+
* the Free Software Foundation, either version 3 of the License, or
8+
* (at your option) any later version.
9+
*
10+
* This program is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13+
* GNU General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU General Public License
16+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
17+
*/
18+
19+
#include "singleexecutor.h"
20+
21+
#include "cppcheck.h"
22+
#include "importproject.h"
23+
#include "library.h"
24+
#include "settings.h"
25+
26+
#include <list>
27+
#include <numeric>
28+
#include <utility>
29+
30+
class ErrorLogger;
31+
32+
SingleExecutor::SingleExecutor(CppCheck &cppcheck, const std::map<std::string, std::size_t> &files, Settings &settings, ErrorLogger &errorLogger)
33+
: Executor(files, settings, errorLogger)
34+
, mCppcheck(cppcheck)
35+
{}
36+
37+
SingleExecutor::~SingleExecutor()
38+
{}
39+
40+
// TODO: markup handling is not performed with multiple jobs
41+
unsigned int SingleExecutor::check()
42+
{
43+
unsigned int result = 0;
44+
45+
const std::size_t totalfilesize = std::accumulate(mFiles.cbegin(), mFiles.cend(), std::size_t(0), [](std::size_t v, const std::pair<std::string, std::size_t>& f) {
46+
return v + f.second;
47+
});
48+
49+
std::size_t processedsize = 0;
50+
unsigned int c = 0;
51+
if (mSettings.project.fileSettings.empty()) {
52+
for (std::map<std::string, std::size_t>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
53+
if (!mSettings.library.markupFile(i->first)
54+
|| !mSettings.library.processMarkupAfterCode(i->first)) {
55+
result += mCppcheck.check(i->first);
56+
processedsize += i->second;
57+
if (!mSettings.quiet)
58+
reportStatus(c + 1, mFiles.size(), processedsize, totalfilesize);
59+
c++;
60+
}
61+
}
62+
} else {
63+
// filesettings
64+
// check all files of the project
65+
for (const ImportProject::FileSettings &fs : mSettings.project.fileSettings) {
66+
result += mCppcheck.check(fs);
67+
++c;
68+
if (!mSettings.quiet)
69+
reportStatus(c, mSettings.project.fileSettings.size(), c, mSettings.project.fileSettings.size());
70+
if (mSettings.clangTidy)
71+
mCppcheck.analyseClangTidy(fs);
72+
}
73+
}
74+
75+
// second loop to parse all markup files which may not work until all
76+
// c/cpp files have been parsed and checked
77+
for (std::map<std::string, std::size_t>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
78+
if (mSettings.library.markupFile(i->first) && mSettings.library.processMarkupAfterCode(i->first)) {
79+
result += mCppcheck.check(i->first);
80+
processedsize += i->second;
81+
if (!mSettings.quiet)
82+
reportStatus(c + 1, mFiles.size(), processedsize, totalfilesize);
83+
c++;
84+
}
85+
}
86+
if (mCppcheck.analyseWholeProgram())
87+
result++;
88+
89+
return result;
90+
}

0 commit comments

Comments
 (0)