Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scriptcomp: Replace all adhoc vector implementations with std::vector #82

Open
mtijanic opened this issue Jul 17, 2023 · 2 comments
Open
Assignees

Comments

@mtijanic
Copy link
Collaborator

Just code cleanup, we have a lot of cases such as:

		// Add the case statement to the list.
		if (m_nSwitchLabelNumber >= m_nSwitchLabelArraySize)
		{
			int32_t *pNewIntArray = new int32_t[m_nSwitchLabelArraySize * 2];
			for (nCount = 0; nCount < m_nSwitchLabelNumber; ++nCount)
			{
				pNewIntArray[nCount] = m_pnSwitchLabelStatements[nCount];
			}
			m_nSwitchLabelArraySize *= 2;
			delete[] m_pnSwitchLabelStatements;
			m_pnSwitchLabelStatements = pNewIntArray;
		}

This should all go away and m_pnSwitchLabelStatements should be a std::vector
Some such cases:

  • m_pnSwitchLabelStatements
  • m_pchResolvedOutputBuffer
  • m_pSymbolLabelList
  • m_pSymbolQueryList
  • m_pchDebuggerCode
  • m_pSRStack

There's probably a good 300 lines of code to just be deleted with this; and makes it less error prone.

@fvinke
Copy link

fvinke commented Jul 17, 2023

I'll handle it, unless someone can find a reason for this not to be cleaned up. For those unfamiliar with my name, PastorPug on discord.

@fvinke
Copy link

fvinke commented Jul 17, 2023

When looking for more arrays that could be vectors I came across m_pcKeyWords, an array of type CScriptCompilerKeyWordEntry. The upper bounds of its size is defined by CSCRIPTCOMPILER_MAX_KEYWORDS.

CScriptCompilerKeyWordEntry is defined as so.

class CScriptCompilerKeyWordEntry
{
public:
	CScriptCompilerKeyWordEntry()
	{
		m_sAlphanumericName = "";
		m_nHashValue = 0;
		m_nNameLength = 0;
		m_nTokenToTranslate = 0;
	}

	void Add(CExoString sName, int32_t nHashValue, int32_t nTokenToTranslate)
	{
        EXOASSERT(m_nHashValue == 0);
		m_sAlphanumericName = sName;
		m_nHashValue = nHashValue;
		m_nNameLength = sName.GetLength();
		m_nTokenToTranslate = nTokenToTranslate;
	}

	inline CExoString *GetPointerToName() { return &m_sAlphanumericName; }
	inline char *GetAlphanumericName() { return m_sAlphanumericName.CStr(); }
	inline uint32_t GetHash() { return m_nHashValue; }
	inline uint32_t GetLength() { return m_nNameLength; }
	inline int32_t   GetTokenToTranslate() { return m_nTokenToTranslate; }

private:
	CExoString m_sAlphanumericName;
	uint32_t      m_nHashValue;
	uint32_t      m_nNameLength;
	int32_t        m_nTokenToTranslate;
};

Keywords are added as such:

m_pcKeyWords = new CScriptCompilerKeyWordEntry[CSCRIPTCOMPILER_MAX_KEYWORDS];
m_pcKeyWords[0] .Add("if"     ,HashString("if"),    CSCRIPTCOMPILER_TOKEN_KEYWORD_IF);
(...)

and added to the hash manager as such:

int32_t nCount;
for (nCount = 0; nCount < CSCRIPTCOMPILER_MAX_KEYWORDS; ++nCount)
{
	HashManagerAdd(CSCRIPTCOMPILER_HASH_MANAGER_TYPE_KEYWORD,nCount);
}

Now here's the problem. Outside of the fact that CScriptCompilerKeyWordEntry in itself is rather questionable, the class seems to be largely redundant. GetPointerToName() is referenced once, GetAlphanumericName() is referenced twice, GetHash() never (effectively meaning that when you add a key you're adding a HashString that is never used), GetLength() never, GetTokenToTranslate() once.

This entire section can be rewritten as a single vector and a struct with two fields (the string and token). Appending to the hash manager can similarly be done as a loop over a vector rather than maintaining a constant separately.

The question is whether this specific case is the domain of this issue. The discord server had talks about overloading functions, which would involve changing up the hash manager (and in turn, likely the keywords), hence I could leave this untouched for the person that will eventually work on that. Alternatively I could spend the 10 minutes required to rework this regardless.

As an aside, pragmatically speaking I'm quite sure writing this took me substantially more time than simply having a piece of code accepted or rejected in a pull request, but I assume it's good manners to bring things like this up. Would it be preferred if I raised issues like this or do I have a go-ahead signal on going potentially slightly out of the scope of the containing issue and resolve the matter during the pull request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants