Differential D2084 Diff 8929 source/graphics/MapGenerator.cpp

Changeset View

Standalone View

source/graphics/MapGenerator.cpp

Show First 20 Lines • Show All 99 Lines • ▼ Show 20 Lines	void* CMapGeneratorWorker::RunThread(void *data)
// Run map generation scripts		// Run map generation scripts
if (!self->Run() \|\| self->m_Progress > 0)		if (!self->Run() \|\| self->m_Progress > 0)
{		{
// Don't leave progress in an unknown state, if generator failed, set it to -1		// Don't leave progress in an unknown state, if generator failed, set it to -1
std::lock_guard<std::mutex> lock(self->m_WorkerMutex);		std::lock_guard<std::mutex> lock(self->m_WorkerMutex);
self->m_Progress = -1;		self->m_Progress = -1;
}		}

		delete self->m_ScriptInterface;
		vladislavbelovUnsubmitted Not Done Inline Actions It's not equal to `SAFE_DELETE`, you don't do `self->m_ScriptInterface = nullptr`. Is that expected? vladislavbelov: It's not equal to `SAFE_DELETE`, you don't do `self->m_ScriptInterface = nullptr`. Is that…
		elexisAuthorUnsubmitted Done Inline Actions I chose `delete` over `SAFE_DELETE` because it's an error if `self->m_ScriptInterface` is null but it doesn't complain about it. Implied assert. I can change to the macro, but it seems like silently hiding an error and pretending nothing happened. Not setting to null shouldn't be a problem since the thread is exited right after. Or the thread runs forever if the mapscrpt never calls `ExportMap` and never reaches the delete. elexis: I chose `delete` over `SAFE_DELETE` because it's an error if `self->m_ScriptInterface` is null…
		historic_brunoUnsubmitted Not Done Inline Actions We should check for a nullptr in a nicer way than that (not rely on mysterious crashes). According to current coding conventions: If deleting a pointer, and it's not in a destructor, and it's not being immediately assigned a new value, use "SAFE_DELETE(p)" (which is equivalent to "delete p; p = nullptr;") to avoid dangling pointers to deleted memory. I guess it could be argued whether it's essentially in a destructor or not, but I think an assignment is cheap, safer, and might prevent future errors. historic_bruno: We should check for a nullptr in a nicer way than that (not rely on mysterious crashes).
		elexisAuthorUnsubmitted Done Inline Actions We should check for a nullptr in a nicer way than that (not rely on mysterious crashes). Actually it doesn't: delete (p); /* if p == 0, delete is a no-op / \ So SAFE_DELETE is superior indeed. elexis:* > We should check for a nullptr in a nicer way than that (not rely on mysterious crashes).
		historic_brunoUnsubmitted Not Done Inline Actions Sorry, I meant your point that m_ScriptInterface might be null (fail to create somehow). Then if we access it, there will be a crash. Or did you not mean that? historic_bruno: Sorry, I meant your point that m_ScriptInterface might be null (fail to create somehow). Then…
		elexisAuthorUnsubmitted Done Inline Actions I thought `delete null;` crashes, but it doesn't. elexis: I thought `delete null;` crashes, but it doesn't.

// At this point the random map scripts are done running, so the thread has no further purpose		// At this point the random map scripts are done running, so the thread has no further purpose
// and can die. The data will be stored in m_MapData already if successful, or m_Progress		// and can die. The data will be stored in m_MapData already if successful, or m_Progress
// will contain an error value on failure.		// will contain an error value on failure.

return NULL;		return NULL;
}		}

bool CMapGeneratorWorker::Run()		bool CMapGeneratorWorker::Run()
{		{
// We must destroy the ScriptInterface in the same thread because the JSAPI requires that!
// Also we must not be in a request when calling the ScriptInterface destructor, so the autoFree object
// must be instantiated before the request (destructors are called in reverse order of instantiation)
struct AutoFree {
AutoFree(ScriptInterface* p) : m_p(p) {}
~AutoFree() { SAFE_DELETE(m_p); }
ScriptInterface* m_p;
} autoFree(m_ScriptInterface);

JSContext* cx = m_ScriptInterface->GetContext();		JSContext* cx = m_ScriptInterface->GetContext();
JSAutoRequest rq(cx);		JSAutoRequest rq(cx);

m_ScriptInterface->SetCallbackData(static_cast<void*> (this));		m_ScriptInterface->SetCallbackData(static_cast<void*> (this));

// Replace RNG with a seeded deterministic function		// Replace RNG with a seeded deterministic function
m_ScriptInterface->ReplaceNondeterministicRNG(m_MapGenRNG);		m_ScriptInterface->ReplaceNondeterministicRNG(m_MapGenRNG);

▲ Show 20 Lines • Show All 297 Lines • Show Last 20 Lines