Skip to content

Commit

Permalink
fixed #12145 - provided order of source files is not preserved (#5625)
Browse files Browse the repository at this point in the history
  • Loading branch information
@firewave
firewave committed Nov 7, 2023
1 parent 8c63c8c commit bc174c5
Show file tree
Hide file tree
Showing 26 changed files with 211 additions and 103 deletions.
9 changes: 4 additions & 5 deletions cli/cppcheckexecutor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,7 @@ bool CppCheckExecutor::parseFromArgs(Settings &settings, int argc, const char* c
}

if (!pathnames.empty()) {
// TODO: this should be a vector or list so the order is kept
std::map<std::string, std::size_t> files;
std::list<std::pair<std::string, std::size_t>> files;
// TODO: this needs to be inlined into PathMatch as it depends on the underlying filesystem
#if defined(_WIN32)
// For Windows we want case-insensitive path matching
Expand Down Expand Up @@ -286,7 +285,7 @@ int CppCheckExecutor::check_wrapper(CppCheck& cppcheck)
return check_internal(cppcheck);
}

bool CppCheckExecutor::reportSuppressions(const Settings &settings, bool unusedFunctionCheckEnabled, const std::map<std::string, std::size_t> &files, ErrorLogger& errorLogger) {
bool CppCheckExecutor::reportSuppressions(const Settings &settings, bool unusedFunctionCheckEnabled, const std::list<std::pair<std::string, std::size_t>> &files, ErrorLogger& errorLogger) {
const auto& suppressions = settings.nomsg.getSuppressions();
if (std::any_of(suppressions.begin(), suppressions.end(), [](const Suppressions::Suppression& s) {
return s.errorId == "unmatchedSuppression" && s.fileName.empty() && s.lineNumber == Suppressions::Suppression::NO_LINE;
Expand All @@ -295,7 +294,7 @@ bool CppCheckExecutor::reportSuppressions(const Settings &settings, bool unusedF

bool err = false;
if (settings.useSingleJob()) {
for (std::map<std::string, std::size_t>::const_iterator i = files.cbegin(); i != files.cend(); ++i) {
for (std::list<std::pair<std::string, std::size_t>>::const_iterator i = files.cbegin(); i != files.cend(); ++i) {
err |= Suppressions::reportUnmatchedSuppressions(
settings.nomsg.getUnmatchedLocalSuppressions(i->first, unusedFunctionCheckEnabled), errorLogger);
}
Expand Down Expand Up @@ -326,7 +325,7 @@ int CppCheckExecutor::check_internal(CppCheck& cppcheck)
settings.loadSummaries();

std::list<std::string> fileNames;
for (std::map<std::string, std::size_t>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i)
for (std::list<std::pair<std::string, std::size_t>>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i)
fileNames.emplace_back(i->first);
AnalyzerInformation::writeFilesTxt(settings.buildDir, fileNames, settings.userDefines, mFileSettings);
}
Expand Down
6 changes: 3 additions & 3 deletions cli/cppcheckexecutor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@
#include <ctime>
#include <iosfwd>
#include <list>
#include <map>
#include <set>
#include <string>
#include <utility>
#include <vector>

class CppCheck;
Expand Down Expand Up @@ -129,7 +129,7 @@ class CppCheckExecutor : public ErrorLogger {
*/
bool parseFromArgs(Settings &settings, int argc, const char* const argv[]);

static bool reportSuppressions(const Settings &settings, bool unusedFunctionCheckEnabled, const std::map<std::string, std::size_t> &files, ErrorLogger& errorLogger);
static bool reportSuppressions(const Settings &settings, bool unusedFunctionCheckEnabled, const std::list<std::pair<std::string, std::size_t>> &files, ErrorLogger& errorLogger);

/**
* Wrapper around check_internal
Expand Down Expand Up @@ -183,7 +183,7 @@ class CppCheckExecutor : public ErrorLogger {
/**
* Filename associated with size of file
*/
std::map<std::string, std::size_t> mFiles;
std::list<std::pair<std::string, std::size_t>> mFiles;

std::list<FileSettings> mFileSettings;

Expand Down
2 changes: 1 addition & 1 deletion cli/executor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@

struct FileSettings;

Executor::Executor(const std::map<std::string, std::size_t> &files, const std::list<FileSettings>& fileSettings, const Settings &settings, Suppressions &suppressions, ErrorLogger &errorLogger)
Executor::Executor(const std::list<std::pair<std::string, std::size_t>> &files, const std::list<FileSettings>& fileSettings, const Settings &settings, Suppressions &suppressions, ErrorLogger &errorLogger)
: mFiles(files), mFileSettings(fileSettings), mSettings(settings), mSuppressions(suppressions), mErrorLogger(errorLogger)
{
// the two inputs may only be used exclusively
Expand Down
6 changes: 3 additions & 3 deletions cli/executor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@

#include <cstddef>
#include <list>
#include <map>
#include <mutex>
#include <string>
#include <utility>

class Settings;
class ErrorLogger;
Expand All @@ -40,7 +40,7 @@ struct FileSettings;
*/
class Executor {
public:
Executor(const std::map<std::string, std::size_t> &files, const std::list<FileSettings>& fileSettings, const Settings &settings, Suppressions &suppressions, ErrorLogger &errorLogger);
Executor(const std::list<std::pair<std::string, std::size_t>> &files, const std::list<FileSettings>& fileSettings, const Settings &settings, Suppressions &suppressions, ErrorLogger &errorLogger);
virtual ~Executor() = default;

Executor(const Executor &) = delete;
Expand All @@ -66,7 +66,7 @@ class Executor {
*/
bool hasToLog(const ErrorMessage &msg);

const std::map<std::string, std::size_t> &mFiles;
const std::list<std::pair<std::string, std::size_t>> &mFiles;
const std::list<FileSettings>& mFileSettings;
const Settings &mSettings;
Suppressions &mSuppressions;
Expand Down
44 changes: 32 additions & 12 deletions cli/filelister.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

#include <cstddef>
#include <cstring>
#include <iterator>
#include <memory>

#ifdef _WIN32
Expand All @@ -39,12 +40,12 @@
// When compiling Unicode targets WinAPI automatically uses *W Unicode versions
// of called functions. Thus, we explicitly call *A versions of the functions.

std::string FileLister::recursiveAddFiles(std::map<std::string, std::size_t> &files, const std::string &path, const std::set<std::string> &extra, const PathMatch& ignored)
std::string FileLister::recursiveAddFiles(std::list<std::pair<std::string, std::size_t>>&files, const std::string &path, const std::set<std::string> &extra, const PathMatch& ignored)
{
return addFiles(files, path, extra, true, ignored);
}

std::string FileLister::addFiles(std::map<std::string, std::size_t> &files, const std::string &path, const std::set<std::string> &extra, bool recursive, const PathMatch& ignored)
std::string FileLister::addFiles(std::list<std::pair<std::string, std::size_t>>&files, const std::string &path, const std::set<std::string> &extra, bool recursive, const PathMatch& ignored)
{
if (path.empty())
return "no path specified";
Expand Down Expand Up @@ -112,18 +113,27 @@ std::string FileLister::addFiles(std::map<std::string, std::size_t> &files, cons

// Limitation: file sizes are assumed to fit in a 'size_t'
#ifdef _WIN64
files[nativename] = (static_cast<std::size_t>(ffd.nFileSizeHigh) << 32) | ffd.nFileSizeLow;
files.emplace_back(nativename, (static_cast<std::size_t>(ffd.nFileSizeHigh) << 32) | ffd.nFileSizeLow);
#else
files[nativename] = ffd.nFileSizeLow;
files.emplace_back(nativename, ffd.nFileSizeLow);
#endif
}
} else {
// Directory
if (recursive) {
if (!ignored.match(fname)) {
std::string err = FileLister::recursiveAddFiles(files, fname, extra, ignored);
std::list<std::pair<std::string, std::size_t>> filesSorted;

std::string err = FileLister::recursiveAddFiles(filesSorted, fname, extra, ignored);
if (!err.empty())
return err;

// files inside directories need to be sorted as the filesystem doesn't provide a stable order
filesSorted.sort([](const decltype(filesSorted)::value_type& a, const decltype(filesSorted)::value_type& b) {
return a.first < b.first;
});

files.insert(files.end(), std::make_move_iterator(filesSorted.begin()), std::make_move_iterator(filesSorted.end()));
}
}
}
Expand Down Expand Up @@ -155,7 +165,7 @@ std::string FileLister::addFiles(std::map<std::string, std::size_t> &files, cons
#include <sys/stat.h>
#include <cerrno>

static std::string addFiles2(std::map<std::string, std::size_t> &files,
static std::string addFiles2(std::list<std::pair<std::string, std::size_t>> &files,
const std::string &path,
const std::set<std::string> &extra,
bool recursive,
Expand All @@ -178,6 +188,8 @@ static std::string addFiles2(std::map<std::string, std::size_t> &files,
std::string new_path = path;
new_path += '/';

std::list<std::pair<std::string, std::size_t>> filesSorted;

while (const dirent* dir_result = readdir(dir)) {
if ((std::strcmp(dir_result->d_name, ".") == 0) ||
(std::strcmp(dir_result->d_name, "..") == 0))
Expand All @@ -193,34 +205,42 @@ static std::string addFiles2(std::map<std::string, std::size_t> &files,
#endif
if (path_is_directory) {
if (recursive && !ignored.match(new_path)) {
std::string err = addFiles2(files, new_path, extra, recursive, ignored);
std::string err = addFiles2(filesSorted, new_path, extra, recursive, ignored);
if (!err.empty()) {
return err;
}
}
} else {
if (Path::acceptFile(new_path, extra) && !ignored.match(new_path)) {
if (stat(new_path.c_str(), &file_stat) != -1)
files[new_path] = file_stat.st_size;
if (stat(new_path.c_str(), &file_stat) != -1) {
filesSorted.emplace_back(new_path, file_stat.st_size);
}
else {
const int err = errno;
return "could not stat file '" + new_path + "' (errno: " + std::to_string(err) + ")";
}
}
}
}

// files inside directories need to be sorted as the filesystem doesn't provide a stable order
filesSorted.sort([](const decltype(filesSorted)::value_type& a, const decltype(filesSorted)::value_type& b) {
return a.first < b.first;
});

files.insert(files.end(), std::make_move_iterator(filesSorted.begin()), std::make_move_iterator(filesSorted.end()));
} else
files[path] = file_stat.st_size;
files.emplace_back(path, file_stat.st_size);
}
return "";
}

std::string FileLister::recursiveAddFiles(std::map<std::string, std::size_t> &files, const std::string &path, const std::set<std::string> &extra, const PathMatch& ignored)
std::string FileLister::recursiveAddFiles(std::list<std::pair<std::string, std::size_t>> &files, const std::string &path, const std::set<std::string> &extra, const PathMatch& ignored)
{
return addFiles(files, path, extra, true, ignored);
}

std::string FileLister::addFiles(std::map<std::string, std::size_t> &files, const std::string &path, const std::set<std::string> &extra, bool recursive, const PathMatch& ignored)
std::string FileLister::addFiles(std::list<std::pair<std::string, std::size_t>> &files, const std::string &path, const std::set<std::string> &extra, bool recursive, const PathMatch& ignored)
{
if (path.empty())
return "no path specified";
Expand Down
15 changes: 8 additions & 7 deletions cli/filelister.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@
#define filelisterH

#include <cstddef>
#include <map>
#include <list>
#include <set>
#include <string>
#include <utility>

class PathMatch;

Expand All @@ -37,12 +38,12 @@ class FileLister {
* Add source files from given directory and all subdirectries to the
* given map. Only files with accepted extensions
* (*.c;*.cpp;*.cxx;*.c++;*.cc;*.txx) are added.
* @param files output map that associates the size of each file with its name
* @param files output list that associates the size of each file with its name
* @param path root path
* @param ignored ignored paths
* @return On success, an empty string is returned. On error, a error message is returned.
*/
static std::string recursiveAddFiles(std::map<std::string, std::size_t> &files, const std::string &path, const PathMatch& ignored) {
static std::string recursiveAddFiles(std::list<std::pair<std::string, std::size_t>> &files, const std::string &path, const PathMatch& ignored) {
const std::set<std::string> extra;
return recursiveAddFiles(files, path, extra, ignored);
}
Expand All @@ -52,27 +53,27 @@ class FileLister {
* Add source files from given directory and all subdirectries to the
* given map. Only files with accepted extensions
* (*.c;*.cpp;*.cxx;*.c++;*.cc;*.txx) are added.
* @param files output map that associates the size of each file with its name
* @param files output list that associates the size of each file with its name
* @param path root path
* @param extra Extra file extensions
* @param ignored ignored paths
* @return On success, an empty string is returned. On error, a error message is returned.
*/
static std::string recursiveAddFiles(std::map<std::string, std::size_t> &files, const std::string &path, const std::set<std::string> &extra, const PathMatch& ignored);
static std::string recursiveAddFiles(std::list<std::pair<std::string, std::size_t>> &files, const std::string &path, const std::set<std::string> &extra, const PathMatch& ignored);

/**
* @brief (Recursively) add source files to a map.
* Add source files from given directory and all subdirectries to the
* given map. Only files with accepted extensions
* (*.c;*.cpp;*.cxx;*.c++;*.cc;*.txx) are added.
* @param files output map that associates the size of each file with its name
* @param files output list that associates the size of each file with its name
* @param path root path
* @param extra Extra file extensions
* @param recursive Enable recursion
* @param ignored ignored paths
* @return On success, an empty string is returned. On error, a error message is returned.
*/
static std::string addFiles(std::map<std::string, std::size_t> &files, const std::string &path, const std::set<std::string> &extra, bool recursive, const PathMatch& ignored);
static std::string addFiles(std::list<std::pair<std::string, std::size_t>> &files, const std::string &path, const std::set<std::string> &extra, bool recursive, const PathMatch& ignored);
};

/// @}
Expand Down
9 changes: 6 additions & 3 deletions cli/processexecutor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include <cstring>
#include <iostream>
#include <list>
#include <map>
#include <sstream> // IWYU pragma: keep
#include <sys/select.h>
#include <sys/wait.h>
Expand All @@ -60,7 +61,7 @@ enum class Color;
using std::memset;


ProcessExecutor::ProcessExecutor(const std::map<std::string, std::size_t> &files, const std::list<FileSettings>& fileSettings, const Settings &settings, Suppressions &suppressions, ErrorLogger &errorLogger, CppCheck::ExecuteCmdFn executeCommand)
ProcessExecutor::ProcessExecutor(const std::list<std::pair<std::string, std::size_t>> &files, const std::list<FileSettings>& fileSettings, const Settings &settings, Suppressions &suppressions, ErrorLogger &errorLogger, CppCheck::ExecuteCmdFn executeCommand)
: Executor(files, fileSettings, settings, suppressions, errorLogger)
, mExecuteCommand(std::move(executeCommand))
{
Expand Down Expand Up @@ -236,7 +237,7 @@ unsigned int ProcessExecutor::check()
std::map<pid_t, std::string> childFile;
std::map<int, std::string> pipeFile;
std::size_t processedsize = 0;
std::map<std::string, std::size_t>::const_iterator iFile = mFiles.cbegin();
std::list<std::pair<std::string, std::size_t>>::const_iterator iFile = mFiles.cbegin();
std::list<FileSettings>::const_iterator iFileSettings = mFileSettings.cbegin();
for (;;) {
// Start a new child
Expand Down Expand Up @@ -325,7 +326,9 @@ unsigned int ProcessExecutor::check()
std::size_t size = 0;
if (p != pipeFile.end()) {
pipeFile.erase(p);
const std::map<std::string, std::size_t>::const_iterator fs = mFiles.find(name);
const auto fs = std::find_if(mFiles.cbegin(), mFiles.cend(), [&name](const std::pair<std::string, std::size_t>& entry) {
return entry.first == name;
});
if (fs != mFiles.end()) {
size = fs->second;
}
Expand Down
4 changes: 2 additions & 2 deletions cli/processexecutor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@

#include <cstddef>
#include <list>
#include <map>
#include <string>
#include <utility>

class Settings;
class ErrorLogger;
Expand All @@ -41,7 +41,7 @@ struct FileSettings;
*/
class ProcessExecutor : public Executor {
public:
ProcessExecutor(const std::map<std::string, std::size_t> &files, const std::list<FileSettings>& fileSettings, const Settings &settings, Suppressions &suppressions, ErrorLogger &errorLogger, CppCheck::ExecuteCmdFn executeCommand);
ProcessExecutor(const std::list<std::pair<std::string, std::size_t>> &files, const std::list<FileSettings>& fileSettings, const Settings &settings, Suppressions &suppressions, ErrorLogger &errorLogger, CppCheck::ExecuteCmdFn executeCommand);
ProcessExecutor(const ProcessExecutor &) = delete;
void operator=(const ProcessExecutor &) = delete;

Expand Down
6 changes: 3 additions & 3 deletions cli/singleexecutor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@

class ErrorLogger;

SingleExecutor::SingleExecutor(CppCheck &cppcheck, const std::map<std::string, std::size_t> &files, const std::list<FileSettings>& fileSettings, const Settings &settings, Suppressions &suppressions, ErrorLogger &errorLogger)
SingleExecutor::SingleExecutor(CppCheck &cppcheck, const std::list<std::pair<std::string, std::size_t>> &files, const std::list<FileSettings>& fileSettings, const Settings &settings, Suppressions &suppressions, ErrorLogger &errorLogger)
: Executor(files, fileSettings, settings, suppressions, errorLogger)
, mCppcheck(cppcheck)
{
Expand All @@ -50,7 +50,7 @@ unsigned int SingleExecutor::check()
std::size_t processedsize = 0;
unsigned int c = 0;

for (std::map<std::string, std::size_t>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
for (std::list<std::pair<std::string, std::size_t>>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
if (!mSettings.library.markupFile(i->first) || !mSettings.library.processMarkupAfterCode(i->first)) {
result += mCppcheck.check(i->first);
processedsize += i->second;
Expand All @@ -77,7 +77,7 @@ unsigned int SingleExecutor::check()
// second loop to parse all markup files which may not work until all
// c/cpp files have been parsed and checked
// TODO: get rid of duplicated code
for (std::map<std::string, std::size_t>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
for (std::list<std::pair<std::string, std::size_t>>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
if (mSettings.library.markupFile(i->first) && mSettings.library.processMarkupAfterCode(i->first)) {
result += mCppcheck.check(i->first);
processedsize += i->second;
Expand Down
4 changes: 2 additions & 2 deletions cli/singleexecutor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@

#include <cstddef>
#include <list>
#include <map>
#include <string>
#include <utility>

class ErrorLogger;
class Settings;
Expand All @@ -35,7 +35,7 @@ struct FileSettings;
class SingleExecutor : public Executor
{
public:
SingleExecutor(CppCheck &cppcheck, const std::map<std::string, std::size_t> &files, const std::list<FileSettings>& fileSettings, const Settings &settings, Suppressions &suppressions, ErrorLogger &errorLogger);
SingleExecutor(CppCheck &cppcheck, const std::list<std::pair<std::string, std::size_t>> &files, const std::list<FileSettings>& fileSettings, const Settings &settings, Suppressions &suppressions, ErrorLogger &errorLogger);
SingleExecutor(const SingleExecutor &) = delete;
void operator=(const SingleExecutor &) = delete;

Expand Down
Loading

0 comments on commit bc174c5

Please sign in to comment.