Page MenuHomeWildfire Games

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

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

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

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

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.Tue, May 28, 12:18 PM
This revision was automatically updated to reflect the committed changes.
elexis added a subscriber: elexis.Mon, Jun 24, 11:44 PM

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.

I don't think there is much value in having both functions, but renaming to AppendPathnames makes sense.