Skip to content

Commit 1956d91

Browse files
firewavefirewave
committed
FileLister: ensure stable sorting of files
FileLister: small cleanup test/cli/other_test.py: added test for `FileLister`
1 parent b04e254 commit 1956d91

File tree

2 files changed

+152
-65
lines changed

2 files changed

+152
-65
lines changed

‎cli/filelister.cpp‎

Lines changed: 81 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -41,16 +41,8 @@
4141
// When compiling Unicode targets WinAPI automatically uses *W Unicode versions
4242
// of called functions. Thus, we explicitly call *A versions of the functions.
4343

44-
std::string FileLister::recursiveAddFiles(std::list<FileWithDetails>&files, const std::string &path, const std::set<std::string> &extra, const PathMatch& ignored)
44+
static std::string addFiles2(std::list<FileWithDetails>&files, const std::string &path, const std::set<std::string> &extra, bool recursive, const PathMatch& ignored)
4545
{
46-
return addFiles(files, path, extra, true, ignored);
47-
}
48-
49-
std::string FileLister::addFiles(std::list<FileWithDetails>&files, const std::string &path, const std::set<std::string> &extra, bool recursive, const PathMatch& ignored)
50-
{
51-
if (path.empty())
52-
return "no path specified";
53-
5446
const std::string cleanedPath = Path::toNativeSeparators(path);
5547

5648
// basedir is the base directory which is used to form pathnames.
@@ -110,22 +102,24 @@ std::string FileLister::addFiles(std::list<FileWithDetails>&files, const std::st
110102
if ((ffd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) == 0) {
111103
// File
112104
if ((!checkAllFilesInDir || Path::acceptFile(fname, extra)) && !ignored.match(fname)) {
113-
const std::string nativename = Path::fromNativeSeparators(fname);
105+
std::string nativename = Path::fromNativeSeparators(fname);
114106

115107
// Limitation: file sizes are assumed to fit in a 'size_t'
116108
#ifdef _WIN64
117-
files.emplace_back(nativename, (static_cast<std::size_t>(ffd.nFileSizeHigh) << 32) | ffd.nFileSizeLow);
109+
const std::size_t filesize = (static_cast<std::size_t>(ffd.nFileSizeHigh) << 32) | ffd.nFileSizeLow;
118110
#else
119-
files.emplace_back(nativename, ffd.nFileSizeLow);
111+
const std::size_t filesize = ffd.nFileSizeLow;
112+
120113
#endif
114+
files.emplace_back(std::move(nativename), filesize);
121115
}
122116
} else {
123117
// Directory
124118
if (recursive) {
125119
if (!ignored.match(fname)) {
126120
std::list<FileWithDetails> filesSorted;
127121

128-
std::string err = FileLister::recursiveAddFiles(filesSorted, fname, extra, ignored);
122+
std::string err = addFiles2(filesSorted, fname, extra, recursive, ignored);
129123
if (!err.empty())
130124
return err;
131125

@@ -152,6 +146,24 @@ std::string FileLister::addFiles(std::list<FileWithDetails>&files, const std::st
152146
return "";
153147
}
154148

149+
std::string FileLister::addFiles(std::list<FileWithDetails> &files, const std::string &path, const std::set<std::string> &extra, bool recursive, const PathMatch& ignored)
150+
{
151+
if (path.empty())
152+
return "no path specified";
153+
154+
std::list<FileWithDetails> filesSorted;
155+
156+
std::string err = addFiles2(filesSorted, path, extra, recursive, ignored);
157+
158+
// files need to be sorted as the filesystems dosn't provide a stable order
159+
filesSorted.sort([](const FileWithDetails& a, const FileWithDetails& b) {
160+
return a.path() < b.path();
161+
});
162+
files.insert(files.end(), std::make_move_iterator(filesSorted.begin()), std::make_move_iterator(filesSorted.end()));
163+
164+
return err;
165+
}
166+
155167
#else
156168

157169
///////////////////////////////////////////////////////////////////////////////
@@ -177,68 +189,58 @@ static std::string addFiles2(std::list<FileWithDetails> &files,
177189
return "";
178190

179191
struct stat file_stat;
180-
if (stat(path.c_str(), &file_stat) != -1) {
181-
if ((file_stat.st_mode & S_IFMT) == S_IFDIR) {
182-
DIR * dir = opendir(path.c_str());
183-
if (!dir) {
184-
const int err = errno;
185-
return "could not open directory '" + path + "' (errno: " + std::to_string(err) + ")";
186-
}
187-
std::unique_ptr<DIR, decltype(&closedir)> dir_deleter(dir, closedir);
192+
if (stat(path.c_str(), &file_stat) == -1)
193+
return ""; // TODO: return error?
194+
if ((file_stat.st_mode & S_IFMT) != S_IFDIR)
195+
{
196+
files.emplace_back(path, file_stat.st_size);
197+
return "";
198+
}
188199

189-
std::string new_path = path;
190-
new_path += '/';
200+
// process directory entry
201+
202+
DIR * dir = opendir(path.c_str());
203+
if (!dir) {
204+
const int err = errno;
205+
return "could not open directory '" + path + "' (errno: " + std::to_string(err) + ")";
206+
}
207+
std::unique_ptr<DIR, decltype(&closedir)> dir_deleter(dir, closedir);
191208

192-
std::list<FileWithDetails> filesSorted;
209+
std::string new_path = path;
210+
new_path += '/';
193211

194-
while (const dirent* dir_result = readdir(dir)) {
195-
if ((std::strcmp(dir_result->d_name, ".") == 0) ||
196-
(std::strcmp(dir_result->d_name, "..") == 0))
197-
continue;
212+
while (const dirent* dir_result = readdir(dir)) {
213+
if ((std::strcmp(dir_result->d_name, ".") == 0) ||
214+
(std::strcmp(dir_result->d_name, "..") == 0))
215+
continue;
198216

199-
new_path.erase(path.length() + 1);
200-
new_path += dir_result->d_name;
217+
new_path.erase(path.length() + 1);
218+
new_path += dir_result->d_name;
201219

202220
#if defined(_DIRENT_HAVE_D_TYPE) || defined(_BSD_SOURCE)
203-
const bool path_is_directory = (dir_result->d_type == DT_DIR || (dir_result->d_type == DT_UNKNOWN && Path::isDirectory(new_path)));
221+
const bool path_is_directory = (dir_result->d_type == DT_DIR || (dir_result->d_type == DT_UNKNOWN && Path::isDirectory(new_path)));
204222
#else
205-
const bool path_is_directory = Path::isDirectory(new_path);
223+
const bool path_is_directory = Path::isDirectory(new_path);
206224
#endif
207-
if (path_is_directory) {
208-
if (recursive && !ignored.match(new_path)) {
209-
std::string err = addFiles2(filesSorted, new_path, extra, recursive, ignored);
210-
if (!err.empty()) {
211-
return err;
212-
}
213-
}
214-
} else {
215-
if (Path::acceptFile(new_path, extra) && !ignored.match(new_path)) {
216-
if (stat(new_path.c_str(), &file_stat) != -1) {
217-
filesSorted.emplace_back(new_path, file_stat.st_size);
218-
}
219-
else {
220-
const int err = errno;
221-
return "could not stat file '" + new_path + "' (errno: " + std::to_string(err) + ")";
222-
}
223-
}
225+
if (path_is_directory) {
226+
if (recursive && !ignored.match(new_path)) {
227+
std::string err = addFiles2(files, new_path, extra, recursive, ignored);
228+
if (!err.empty()) {
229+
return err;
224230
}
225231
}
226-
227-
// files inside directories need to be sorted as the filesystem doesn't provide a stable order
228-
filesSorted.sort([](const FileWithDetails& a, const FileWithDetails& b) {
229-
return a.path() < b.path();
230-
});
231-
232-
files.insert(files.end(), std::make_move_iterator(filesSorted.begin()), std::make_move_iterator(filesSorted.end()));
233-
} else
234-
files.emplace_back(path, file_stat.st_size);
232+
} else {
233+
if (Path::acceptFile(new_path, extra) && !ignored.match(new_path)) {
234+
if (stat(new_path.c_str(), &file_stat) == -1) {
235+
const int err = errno;
236+
return "could not stat file '" + new_path + "' (errno: " + std::to_string(err) + ")";
237+
}
238+
files.emplace_back(new_path, file_stat.st_size);
239+
}
240+
}
235241
}
236-
return "";
237-
}
238242

239-
std::string FileLister::recursiveAddFiles(std::list<FileWithDetails> &files, const std::string &path, const std::set<std::string> &extra, const PathMatch& ignored)
240-
{
241-
return addFiles(files, path, extra, true, ignored);
243+
return "";
242244
}
243245

244246
std::string FileLister::addFiles(std::list<FileWithDetails> &files, const std::string &path, const std::set<std::string> &extra, bool recursive, const PathMatch& ignored)
@@ -250,7 +252,22 @@ std::string FileLister::addFiles(std::list<FileWithDetails> &files, const std::s
250252
if (endsWith(corrected_path, '/'))
251253
corrected_path.erase(corrected_path.end() - 1);
252254

253-
return addFiles2(files, corrected_path, extra, recursive, ignored);
255+
std::list<FileWithDetails> filesSorted;
256+
257+
std::string err = addFiles2(filesSorted, corrected_path, extra, recursive, ignored);
258+
259+
// files need to be sorted as the filesystems dosn't provide a stable order
260+
filesSorted.sort([](const FileWithDetails& a, const FileWithDetails& b) {
261+
return a.path() < b.path();
262+
});
263+
files.insert(files.end(), std::make_move_iterator(filesSorted.begin()), std::make_move_iterator(filesSorted.end()));
264+
265+
return err;
254266
}
255267

256268
#endif
269+
270+
std::string FileLister::recursiveAddFiles(std::list<FileWithDetails> &files, const std::string &path, const std::set<std::string> &extra, const PathMatch& ignored)
271+
{
272+
return addFiles(files, path, extra, true, ignored);
273+
}

‎test/cli/other_test.py‎

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1357,4 +1357,74 @@ def test_rule(tmpdir):
13571357
lines = stderr.splitlines()
13581358
assert lines == [
13591359
"{}:4:0: style: found 'f' [rule]".format(test_file)
1360-
]
1360+
]
1361+
1362+
1363+
def test_filelist(tmpdir):
1364+
list_dir = os.path.join(tmpdir, 'list-dir')
1365+
os.mkdir(list_dir)
1366+
1367+
with open(os.path.join(list_dir, 'aaa.c'), 'wt'):
1368+
pass
1369+
with open(os.path.join(list_dir, 'zzz.c'), 'wt'):
1370+
pass
1371+
with open(os.path.join(list_dir, 'valueflow.cpp'), 'wt'):
1372+
pass
1373+
with open(os.path.join(list_dir, 'vfvalue.cpp'), 'wt'):
1374+
pass
1375+
with open(os.path.join(list_dir, 'vf_enumvalue.cpp'), 'wt'):
1376+
pass
1377+
with open(os.path.join(list_dir, 'vf_analyze.h'), 'wt'):
1378+
pass
1379+
1380+
sub_dir_1 = os.path.join(list_dir, 'valueflow')
1381+
os.mkdir(sub_dir_1)
1382+
with open(os.path.join(sub_dir_1, 'file.cpp'), 'wt'):
1383+
pass
1384+
with open(os.path.join(sub_dir_1, 'file.c'), 'wt'):
1385+
pass
1386+
with open(os.path.join(sub_dir_1, 'file.h'), 'wt'):
1387+
pass
1388+
1389+
sub_dir_2 = os.path.join(list_dir, 'vfvalue')
1390+
os.mkdir(sub_dir_2)
1391+
with open(os.path.join(sub_dir_2, 'file.cpp'), 'wt'):
1392+
pass
1393+
with open(os.path.join(sub_dir_2, 'file.c'), 'wt'):
1394+
pass
1395+
with open(os.path.join(sub_dir_2, 'file.h'), 'wt'):
1396+
pass
1397+
1398+
sub_dir_3 = os.path.join(list_dir, 'vf_enumvalue')
1399+
os.mkdir(sub_dir_3)
1400+
with open(os.path.join(sub_dir_3, 'file.cpp'), 'wt'):
1401+
pass
1402+
with open(os.path.join(sub_dir_3, 'file.c'), 'wt'):
1403+
pass
1404+
with open(os.path.join(sub_dir_3, 'file.h'), 'wt'):
1405+
pass
1406+
1407+
# TODO: -rp is not applied to "Checking" messages
1408+
#exitcode, stdout, _ = cppcheck(['-j1', '-rp', list_dir])
1409+
exitcode, stdout, _ = cppcheck(['-j1', '.'], cwd=list_dir)
1410+
assert exitcode == 0, stdout
1411+
if sys.platform == "win32":
1412+
stdout = stdout.replace('\\', '/')
1413+
lines = stdout.splitlines()
1414+
expected = [
1415+
'Checking aaa.c ...',
1416+
'Checking valueflow.cpp ...',
1417+
'Checking valueflow/file.c ...',
1418+
'Checking valueflow/file.cpp ...',
1419+
'Checking vf_enumvalue.cpp ...',
1420+
'Checking vf_enumvalue/file.c ...',
1421+
'Checking vf_enumvalue/file.cpp ...',
1422+
'Checking vfvalue.cpp ...',
1423+
'Checking vfvalue/file.c ...',
1424+
'Checking vfvalue/file.cpp ...',
1425+
'Checking zzz.c ...'
1426+
]
1427+
assert len(expected), len(lines)
1428+
for i in range(1, len(expected)+1):
1429+
lines.remove('{}/11 files checked 0% done'.format(i, len(expected)))
1430+
assert lines == expected

0 commit comments

Comments
 (0)