this file needs rebase following https://code.wildfiregames.com/rP19200#5c8659fc

(So reverting that commit and then rebasing the patch is actually easier)

That doesn't need to be global, only in the scope that uses it (see my proposal for examplE).

g_Foo only used for actual globals afaik and it won't be accessible unless declaring it with the external keyword and it will be impossible when using the static keyword afaik, so +static -g_

Can be removed IMO, too noisy, only opening of one file (which may hang if the HDD hangs. Still nothing we can do about that with code then.)
Since the performance effects of file reading should be well understood in theory, I don't see any other performance issues arising here anytime soon.

not really needed

We only need one of those 2 comments.

The JSON is parsed. The context is the spider monkey environment which stores the currently known variables and functions. Comment doesn't really state anything besides that what is known from the function title.

LOGERROR should be used for serious things. Use LOGWARNING if you insist on notifying the user (which doesn't really seem necessary to me since noone will legitimately edit that file. It might only break when writing it while the disk has no more space left.).

Same.

Unneeded and noisy too IMO

Looks like you could avoid duplication here by not inverting the conditions.

As far as I'm concerned it is good practice to move hardcoded constants to the top of the file.

That error seems more legitimate, since the file system couldn't be written to.

The replays aren't updated, only the cache.

(
Since this is only called locally, could be made 'private' of that file (i.e. not a namespace 'member' like goBackToLineBeginning). But that will have the ugly side-effect of having to call VisualReplay::GetDirectoryName() instead of GetDirectoryName() and technically it could be called from outside for any reason.

Merging those 2 functions doesn't seem to be nicely doable because this one contains return statements which would have to be compensated with ugly scopes.
)

That can be kept since it is guaranteed to consume a lot of time if many unknown replays are added.

Comment not needed anymore when using the readable types

Since the type is used at least twice, a typedef would be nice, like typedef std::map<OsPath, std::pair<u32, off_t>> replayCacheMap;

duplicate \n

Unneeded, noisy

Correct to use u32, because 2^32-1 is the greatest number of elements a JS array can hold.

This C++11 doesn't appear to be saving anything in comparison to fileList[fileName] = ...

// Specifies whether the next replay file should be kept

Not needed

Why this scope? seems unneeded. Sounds like it came from having had a TIMER in there

Why is the filesize u32? It should be double so that we are not bound to 4GB but to 2^53 IMO and it's consistent with the other places.

If I recall correctly, we had talked about the use case of having a break instead of return here long before someone suggested to use a return here. The idea is that if the user has tons of replays and an old school harddisk with files scattered wildly, he can wait 30 seconds for replays to load, then press Alt+F4 after losing patience and then not lose the passed waiting time as the progress has been saved. If we return immediately, the progress will be lost.
Saving the file will take only a fraction of a second, so it's not relevant to return immediately.

You might also recall the course of action that we take when changing code back to an earlier form because we forgot an (hint: undocumented) edge case / use case.

We

don't want to lose -> want to save
pretty fast part not needed

u32 filesize:
Better use the actual type returned than converting things needlessly (and assuming it never encounters a replay file larger than 4GB).

In file_system.h we see the filesize returned is of the type off_t, not exactly specified by posix.

Using std::time_t and then converting it to double in the scriptInterface.SetProperty calls is what we do in the savegame & replay timestamp code, because double is equivalent to the only JS number type used. The greatest number is 2^53 iirc (i.e. holds much more than 2^32, but still less than 2^64).

ps/Replay.cpp:	m_ScriptInterface.SetProperty(attribs, "timestamp", (double)std::time(nullptr));
ps/SavedGame.cpp:	simulation.GetScriptInterface().SetProperty(metadata, "time", (double)now);

should be self-evident

That commands.txt might be moved to a const as well (wchar_t array I guess).

Remove the conversion then

Aren't these Eval calls creating empty objects a cheap copy of foo = JS_NewArrayObject(cx, 0)? https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_NewArrayObject

Didn't we say something about the 4GB limitation and what c++ type resembles the JS number type?

Hard to read with only 2 spaces replacing tabs, but wouldn't we save one line if this is false by default and only set to true if the conditions are met? (Even merging two if statements?)

zu is both the correcŧ entry for a printf using std::size_t and an interesting band to recommend while at it.

Three lines feel a bit noisy, why not all in one:

	debug_printf(
		"Loading %zu cached replays, removed %zu outdated entries, loaded %i new entries\n",
		fileList.size(),
		fileList.size() - copyFromOldCache.size(),
		i);

\n after return

not

unneeded comment

How about not adding files without attribs instead of removing them?

replayNonEmpty?

Remove this TIMER call, since we want only one call for the loading of all replay files, not one for each IMO

Use (double), the whole JS number type parts takes place at D205

Since this is only called when ending the game, maybe.