Don't clear pathnames in vfs::GetPathnames so it can be called several times.
ClosedPublic
Actions

Authored by wraitii on May 25 2019, 12:53 PM.

Details

Reviewers

None

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP22312: Don't clear pathnames in vfs::GetPathnames so it can be called several times.

Summary

vfs::GetPathnames currently clears the pathnames vector to which it appends path names. This is useless because we always create the vector right before calling GetPathnames, and it makes it annoying to repeatedly call GetPathnames (see D1858 for a possible use case).

This adds tests to make sure we don't break the behaviour at some point.

Test Plan

grep for GetPathnames, notice that this is safe.
review tests

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

wraitii created this revision.May 25 2019, 12:53 PM

wraitii added a child revision: D1858: Add a MultiKeyMap helper and tests (+ allow tests in /helpers) - D274 prereq.

wraitii mentioned this in D1858: Add a MultiKeyMap helper and tests (+ allow tests in /helpers) - D274 prereq.May 25 2019, 12:57 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1476/display/redirect

Harbormaster completed remote builds in B7576: Diff 8114.May 25 2019, 1:05 PM

Stan added a subscriber: Stan.May 25 2019, 1:09 PM

Stan added inline comments.

source/lib/file/vfs/tests/test_vfs_util.h
29 ↗	(On Diff #8114)	Can't they be static to the class instead ? Even though they are tests.
85 ↗	(On Diff #8114)	Can you delete the files afterwards so unlike thr other tests it doesnt keep a lot of bloat around ?

wraitii added inline comments.May 25 2019, 1:16 PM

source/lib/file/vfs/tests/test_vfs_util.h
29 ↗	(On Diff #8114)	would need to initialise them as `static OsPath MOD_PATH = OsPath(DataDir()/"mods"/"_test.vfs");` which annoyed me enough to not do it.
85 ↗	(On Diff #8114)	I'm fairly sure deleting the directory (done in `cleanup()`) deletes the files.

This revision was not accepted when it landed; it landed in state Needs Review.May 28 2019, 12:18 PM

Closed by commit rP22312: Don't clear pathnames in vfs::GetPathnames so it can be called several times. (authored by wraitii). · Explain Why

This revision was automatically updated to reflect the committed changes.

grep for GetPathnames, notice that this is safe.

Performed, confirmed grep -R 'GetPathnames' -B1

i18n/L10n.cpp-	std::wstring dictName = GetFallbackToAvailableDictLocale(icu::Locale::createCanonical(locale.c_str()));
i18n/L10n.cpp:	vfs::GetPathnames(g_VFS, L"l10n/", dictName.append(L".*.po").c_str(), filenames);
--
i18n/L10n.cpp-	{
i18n/L10n.cpp:		if (vfs::GetPathnames(g_VFS, L"l10n/", L"long.*.po", filenames) < 0)
--
i18n/L10n.cpp-		std::wstring dictName = GetFallbackToAvailableDictLocale(currentLocale);
i18n/L10n.cpp:		if (vfs::GetPathnames(g_VFS, L"l10n/", dictName.append(L".*.po").c_str(), filenames) < 0)
--
i18n/L10n.cpp-	VfsPaths filenames;
i18n/L10n.cpp:	if (vfs::GetPathnames(g_VFS, L"l10n/", L"*.po", filenames) < 0)
--
graphics/MapGenerator.cpp-	// Load all scripts in mapgen directory
graphics/MapGenerator.cpp:	Status ret = vfs::GetPathnames(g_VFS, path, L"*.js", pathnames);
--
graphics/ColladaManager.cpp-		VfsPaths pathnames;
graphics/ColladaManager.cpp:		if (vfs::GetPathnames(m_VFS, L"art/skeletons/", L"*.xml", pathnames) < 0)
--
graphics/ColladaManager.cpp-			VfsPaths paths;
graphics/ColladaManager.cpp:			if (vfs::GetPathnames(m_VFS, L"art/skeletons/", L"*.xml", paths) != INFO::OK)
--
renderer/PostprocManager.cpp-	VfsPaths pathnames;
renderer/PostprocManager.cpp:	if (vfs::GetPathnames(g_VFS, path, 0, pathnames) < 0)
--
gui/GUIManager.cpp-			VfsPaths pathnames;
gui/GUIManager.cpp:			vfs::GetPathnames(g_VFS, directory, L"*.xml", pathnames);
--
gui/CGUI.cpp-				VfsPaths pathnames;
gui/CGUI.cpp:				vfs::GetPathnames(g_VFS, directory, L"*.xml", pathnames);
--
gui/CGUI.cpp-		VfsPaths pathnames;
gui/CGUI.cpp:		vfs::GetPathnames(g_VFS, directory, L"*.js", pathnames);
--
scriptinterface/ScriptInterface.cpp-	VfsPaths pathnames;
scriptinterface/ScriptInterface.cpp:	vfs::GetPathnames(g_VFS, L"globalscripts/", L"*.js", pathnames);
--
lib/file/vfs/vfs_util.cpp-
lib/file/vfs/vfs_util.cpp:Status GetPathnames(const PIVFS& fs, const VfsPath& path, const wchar_t* filter, VfsPaths& pathnames)
--
lib/file/vfs/vfs_util.h-
lib/file/vfs/vfs_util.h:extern Status GetPathnames(const PIVFS& fs, const VfsPath& path, const wchar_t* filter, VfsPaths& pathnames);
--
lib/file/vfs/tests/test_vfs_util.h-		VfsPaths pathNames;
lib/file/vfs/tests/test_vfs_util.h:		vfs::GetPathnames(g_VFS, "", L"*.txt", pathNames);
lib/file/vfs/tests/test_vfs_util.h-		TS_ASSERT_EQUALS(pathNames.size(), 3);
lib/file/vfs/tests/test_vfs_util.h:		vfs::GetPathnames(g_VFS, "sub_folder_a/", L"*.txt", pathNames);
lib/file/vfs/tests/test_vfs_util.h-		TS_ASSERT_EQUALS(pathNames.size(), 5);
lib/file/vfs/tests/test_vfs_util.h:		vfs::GetPathnames(g_VFS, "sub_folder_b/", L"*.txt", pathNames);
--
simulation2/Simulation2.cpp-	VfsPaths pathnames;
simulation2/Simulation2.cpp:	if (vfs::GetPathnames(g_VFS, path, L"*.js", pathnames) < 0)
--
simulation2/Simulation2.cpp-	VfsPaths pathnames;
simulation2/Simulation2.cpp:	Status ret = vfs::GetPathnames(g_VFS, path, L"*.json", pathnames);
--
simulation2/components/CCmpAIManager.cpp-		VfsPaths pathnames;
simulation2/components/CCmpAIManager.cpp:		if (vfs::GetPathnames(g_VFS, L"simulation/ai/" + moduleName + L"/", L"*.js", pathnames) < 0)
--
simulation2/components/tests/test_scripts.h-		VfsPaths paths;
simulation2/components/tests/test_scripts.h:		TS_ASSERT_OK(vfs::GetPathnames(g_VFS, L"simulation/components/tests/", L"test_*.js", paths));
--
ps/SavedGame.cpp-	VfsPaths pathnames;
ps/SavedGame.cpp:	err = vfs::GetPathnames(g_VFS, "saves/", L"*.0adsave", pathnames);

L10n.cpp, MapGenerator.cpp independently confirmed.

I personally would not have changed GetPathnames to append to the existing array, at least it would be good if that would be declared somewhere.

It seems the primary motivation for this commit was to avoid a concatenation in your D1858 proposal:

		VfsPaths paths;
		TS_ASSERT_OK(vfs::GetPathnames(g_VFS, L"simulation/components/tests/", L"test_*.js", paths));
		TS_ASSERT_OK(vfs::GetPathnames(g_VFS, L"simulation/helpers/tests/", L"test_*.js", paths));

I guess that's not use case to be dismiss, as storing it in a temporary variable and concatenating afterwards wold be ugly, and to create a for-loop over these two folders would also be ugly.

Perhaps this would be cleaner:

		VfsPaths paths;
		TS_ASSERT_OK(vfs::AppendPathnames(g_VFS, L"simulation/components/tests/", L"test_*.js", paths));
		TS_ASSERT_OK(vfs::AppendPathnames(g_VFS, L"simulation/helpers/tests/", L"test_*.js", paths));

where AppendPathnames is the GetPathnames function without the clear and GetPathnames is a new function that does perform the clear and then the append.
This way one can retain the thought that a GetFoo writes the result to a variable (i.e. clears) and has the use case covered at the same time.