Skip to content
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

fixed #12145 - provided order of source files is not preserved #5625

Merged
merged 3 commits into from
Nov 7, 2023
Merged

fixed #12145 - provided order of source files is not preserved #5625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8196ed9
fixed #12145 - provided order of source files is not preserved
firewave Nov 4, 2023
4ee4077
dmake.vcxproj: fixed linking
firewave Nov 6, 2023
603ac62
checkthread.cpp: fixed `useStlAlgorithm` selfcheck warning
firewave Nov 6, 2023
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
9 changes: 4 additions & 5 deletions cli/cppcheckexecutor.cpp
View file
Open in desktop
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;
danmar marked this conversation as resolved.
Show resolved Hide resolved
// 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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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) {
Copy link
Collaborator Author

@firewave firewave Nov 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The files inside the folder still need to be sorted as the order retrieved from the filesystem is random.

I did not add an explicit test for this as this is implicitly tested by dmake. The order of the files were different in the generated files before I added the sorting.

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading