-
Notifications
You must be signed in to change notification settings - Fork 178
refactor(ini): Make the INI code cleaner and more consistent #2596
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
base: main
Are you sure you want to change the base?
Changes from all commits
712d790
e762fbe
e87afc0
4eeb8d9
b6fb802
8bb5572
61dbf3a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,13 +160,8 @@ typedef void (*BuildMultiIniFieldProc)(MultiIniFieldParse& p); | |
| //------------------------------------------------------------------------------------------------- | ||
| class INI | ||
| { | ||
| INI(const INI&); | ||
| INI& operator=(const INI&); | ||
|
|
||
| public: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The PR removes the private // Suggested fix — add in the public or class body:
INI(const INI&) = delete;
INI& operator=(const INI&) = delete;Prompt To Fix With AIThis is a comment left during a code review.
Path: Core/GameEngine/Include/Common/INI.h
Line: 163
Comment:
**Missing `= delete` for copy/assignment after removing private declarations**
The PR removes the private `INI(const INI&)` / `INI& operator=(const INI&)` declarations without replacing them with `= delete`. `m_readBuffer` is heap-allocated (`file->readEntireAndClose()`) and released via `delete[] m_readBuffer` in `unPrepFile()`. With no copy protection, the compiler will generate a shallow-copy constructor and assignment operator, meaning two INI instances could share the same `m_readBuffer` pointer. When either calls `unPrepFile()`, the other is left with a dangling pointer — use-after-free or double-delete follows.
```cpp
// Suggested fix — add in the public or class body:
INI(const INI&) = delete;
INI& operator=(const INI&) = delete;
```
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| INI(); | ||
| ~INI(); | ||
|
|
||
| // TheSuperHackers @feature xezon 19/08/2025 | ||
| // Load a specific INI file by name and/or INI files from a directory (and its subdirectories). | ||
|
|
@@ -251,11 +246,12 @@ class INI | |
| AsciiString getFilename() const { return m_filename; } | ||
| INILoadType getLoadType() const { return m_loadType; } | ||
| UnsignedInt getLineNum() const { return m_lineNum; } | ||
| const char *getSeps() const { return m_seps; } | ||
| const char *getSepsPercent() const { return m_sepsPercent; } | ||
| const char *getSepsColon() const { return m_sepsColon; } | ||
| const char *getSepsQuote() { return m_sepsQuote; } | ||
| Bool isEOF() const { return m_endOfFile; } | ||
| static const char *getSeps() { return " \n\r\t="; } ///< default delimiters for strtok parsing | ||
| static const char *getSepsPercent() { return " \n\r\t=%%"; } ///< default delimiters & percent delimiter | ||
| static const char *getSepsColon() { return " \n\r\t=:"; } ///< default delimiters & colon delimiter | ||
| static const char *getSepsQuote() { return "\"\n="; } ///< delimiters to represent a quoted ascii string | ||
| static const char *getEndToken() { return "End"; } ///< token to represent an end of data block | ||
|
|
||
| void initFromINI( void *what, const FieldParse* parseTable ); | ||
| void initFromINIMulti( void *what, const MultiIniFieldParse& parseTableList ); | ||
|
|
@@ -321,20 +317,19 @@ class INI | |
| static void parseVeterancyLevelFlags(INI* ini, void* instance, void* store, const void* userData); | ||
| static void parseSoundsList( INI* ini, void *instance, void *store, const void* /*userData*/ ); | ||
|
|
||
|
|
||
| /** | ||
| return the next token. if seps is null (or omitted), the standard seps are used. | ||
| return the next token. if seps is not specified, the standard seps are used. | ||
|
|
||
| this will *never* return null; if there are no more tokens, an exception will be thrown. | ||
| */ | ||
| const char* getNextToken(const char* seps = nullptr); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should only have one token retrieving function and let external code handle an invalid return depending on where the check is being performed, i don't like the idea of it just throwing an error due to a malformed ini, it should more gracefully handle it and warn the user.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, maybe, but would require a larger change. I'm working on a large overhaul of the INI code, but it's a significant endeavor so I'm not sure it'll work out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries, it's just nasty that the original token retrieval code throws in the first place. |
||
| static const char* getNextToken(const char* seps = getSeps()); | ||
|
|
||
| /** | ||
| just like getNextToken(), except that null is returned if no more tokens are present | ||
| (rather than throwing an exception). usually you should call getNextToken(), | ||
| but for some cases this is handier (ie, parsing a variable-length number of tokens). | ||
| */ | ||
| const char* getNextTokenOrNull(const char* seps = nullptr); | ||
| static const char* getNextTokenOrNull(const char* seps = getSeps()); | ||
|
|
||
| /** | ||
| This is called when the next thing you expect is something like: | ||
|
|
@@ -346,7 +341,7 @@ class INI | |
|
|
||
| If "Tag" is not the next token, an error is thrown. | ||
| */ | ||
| const char* getNextSubToken(const char* expected); | ||
| static const char* getNextSubToken(const char* expected); | ||
|
|
||
| /** | ||
| return the next ascii string. this is usually the same the result of getNextToken(), | ||
|
|
@@ -400,14 +395,9 @@ class INI | |
| unsigned m_readBufferUsed; ///< number of bytes in read buffer | ||
|
|
||
| AsciiString m_filename; ///< filename of file currently loading | ||
| INILoadType m_loadType; ///< load time for current file | ||
| INILoadType m_loadType; ///< load type for current file | ||
| UnsignedInt m_lineNum; ///< current line number that's been read | ||
| char m_buffer[ INI_MAX_CHARS_PER_LINE+1 ];///< buffer to read file contents into | ||
| const char *m_seps; ///< for strtok parsing | ||
| const char *m_sepsPercent; ///< m_seps with percent delimiter as well | ||
| const char *m_sepsColon; ///< m_seps with colon delimiter as well | ||
| const char *m_sepsQuote; ///< token to represent a quoted ascii string | ||
| const char *m_blockEndToken; ///< token to represent end of data block | ||
| Bool m_endOfFile; ///< TRUE when we've hit EOF | ||
| #ifdef DEBUG_CRASHING | ||
| char m_curBlockStart[ INI_MAX_CHARS_PER_LINE+1 ]; ///< first line of cur block | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -180,26 +180,14 @@ INI::INI() | |
| m_filename = "None"; | ||
| m_loadType = INI_LOAD_INVALID; | ||
| m_lineNum = 0; | ||
| m_seps = " \n\r\t="; ///< make sure you update m_sepsPercent/m_sepsColon as well | ||
| m_sepsPercent = " \n\r\t=%%"; | ||
| m_sepsColon = " \n\r\t=:"; | ||
| m_sepsQuote = "\"\n="; ///< stop at " = EOL | ||
| m_blockEndToken = "END"; | ||
| m_endOfFile = FALSE; | ||
| m_buffer[0] = 0; | ||
| m_endOfFile = FALSE; | ||
| #ifdef DEBUG_CRASHING | ||
| m_curBlockStart[0] = 0; | ||
| #endif | ||
|
|
||
| } | ||
|
|
||
| //------------------------------------------------------------------------------------------------- | ||
| //------------------------------------------------------------------------------------------------- | ||
| INI::~INI() | ||
| { | ||
|
|
||
| } | ||
|
|
||
| //------------------------------------------------------------------------------------------------- | ||
| UnsignedInt INI::loadFileDirectory( AsciiString fileDirName, INILoadType loadType, Xfer *pXfer, Bool subdirs ) | ||
| { | ||
|
|
@@ -397,7 +385,7 @@ UnsignedInt INI::load( AsciiString filename, INILoadType loadType, Xfer *pXfer ) | |
| AsciiString currentLine = m_buffer; | ||
|
|
||
| // the first word is the type of data we're processing | ||
| const char *token = strtok( m_buffer, m_seps ); | ||
| const char *token = strtok( m_buffer, getSeps() ); | ||
| if( token ) | ||
| { | ||
| INIBlockParse parse = findBlockParse(token); | ||
|
|
@@ -710,7 +698,7 @@ void INI::parseAsciiStringVector( INI* ini, void * /*instance*/, void *store, co | |
| { | ||
| std::vector<AsciiString>* asv = (std::vector<AsciiString>*)store; | ||
| asv->clear(); | ||
| for (const char *token = ini->getNextTokenOrNull(); token != nullptr; token = ini->getNextTokenOrNull()) | ||
| for (const char *token = ini->getNextTokenOrNull(); token; token = ini->getNextTokenOrNull()) | ||
| { | ||
| asv->push_back(token); | ||
| } | ||
|
|
@@ -723,7 +711,7 @@ void INI::parseAsciiStringVectorAppend( INI* ini, void * /*instance*/, void *sto | |
| std::vector<AsciiString>* asv = (std::vector<AsciiString>*)store; | ||
| // nope, don't clear. duh. | ||
| // asv->clear(); | ||
| for (const char *token = ini->getNextTokenOrNull(); token != nullptr; token = ini->getNextTokenOrNull()) | ||
| for (const char *token = ini->getNextTokenOrNull(); token; token = ini->getNextTokenOrNull()) | ||
| { | ||
| asv->push_back(token); | ||
| } | ||
|
|
@@ -735,7 +723,7 @@ void INI::parseAsciiStringVectorAppend( INI* ini, void * /*instance*/, void *sto | |
| { | ||
| ScienceVec* asv = (ScienceVec*)store; | ||
| asv->clear(); | ||
| for (const char *token = ini->getNextTokenOrNull(); token != nullptr; token = ini->getNextTokenOrNull()) | ||
| for (const char *token = ini->getNextTokenOrNull(); token; token = ini->getNextTokenOrNull()) | ||
| { | ||
| if (stricmp(token, "None") == 0) | ||
| { | ||
|
|
@@ -952,7 +940,7 @@ void INI::parseBitString32( INI* ini, void * /*instance*/, void *store, const vo | |
| Bool foundAddOrSub = false; | ||
|
|
||
| // loop through all tokens | ||
| for (const char *token = ini->getNextTokenOrNull(); token != nullptr; token = ini->getNextTokenOrNull()) | ||
| for (const char *token = ini->getNextTokenOrNull(); token; token = ini->getNextTokenOrNull()) | ||
| { | ||
| if (stricmp(token, "NONE") == 0) | ||
| { | ||
|
|
@@ -1528,7 +1516,7 @@ void INI::initFromINIMulti( void *what, const MultiIniFieldParse& parseTableList | |
| if( field ) | ||
| { | ||
|
|
||
| if( stricmp( field, m_blockEndToken ) == 0 ) | ||
| if( stricmp( field, getEndToken() ) == 0 ) | ||
| { | ||
| done = TRUE; | ||
| } | ||
|
|
@@ -1580,7 +1568,7 @@ void INI::initFromINIMulti( void *what, const MultiIniFieldParse& parseTableList | |
|
|
||
| done = TRUE; | ||
| DEBUG_CRASH( ("Error parsing block '%s', in INI file '%s'. Missing '%s' token", | ||
| m_curBlockStart, getFilename().str(), m_blockEndToken) ); | ||
| m_curBlockStart, getFilename().str(), getEndToken()) ); | ||
| throw INI_MISSING_END_TOKEN; | ||
|
|
||
| } | ||
|
|
@@ -1590,19 +1578,17 @@ void INI::initFromINIMulti( void *what, const MultiIniFieldParse& parseTableList | |
| } | ||
|
|
||
| //------------------------------------------------------------------------------------------------- | ||
| /*static*/ const char* INI::getNextToken(const char* seps) | ||
| /*static*/ const char* INI::getNextToken(const char* seps /*= getSeps()*/) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to block comment the default argument. |
||
| { | ||
| if (!seps) seps = getSeps(); | ||
| const char *token = ::strtok(nullptr, seps); | ||
| if (!token) | ||
| throw INI_INVALID_DATA; | ||
| return token; | ||
| } | ||
|
|
||
| //------------------------------------------------------------------------------------------------- | ||
| /*static*/ const char* INI::getNextTokenOrNull(const char* seps) | ||
| /*static*/ const char* INI::getNextTokenOrNull(const char* seps /*= getSeps()*/) | ||
| { | ||
| if (!seps) seps = getSeps(); | ||
| const char *token = ::strtok(nullptr, seps); | ||
| return token; | ||
| } | ||
|
|
@@ -1867,12 +1853,10 @@ void INI::parseSoundsList( INI* ini, void *instance, void *store, const void* /* | |
| std::vector<AsciiString> *vec = (std::vector<AsciiString>*) store; | ||
| vec->clear(); | ||
|
|
||
| const char* SEPS = " \t,="; | ||
| const char *c = ini->getNextTokenOrNull(SEPS); | ||
| while ( c ) | ||
| constexpr const char* SEPS = " \t,="; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or Seps |
||
| for (const char* token = ini->getNextTokenOrNull(SEPS); token; token = ini->getNextTokenOrNull(SEPS)) | ||
| { | ||
| vec->push_back( c ); | ||
| c = ini->getNextTokenOrNull(SEPS); | ||
| vec->push_back(token); | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prob best to keep the copy and copy move constructors private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the INI class is not a simple class, it has internal data handled on the heap.
The default copy constructor will only make a shallow copy of the INI, so if the original is deleted the pointed at memory in m_framebuffer will dangle to invalid memory.
the manual destructor tbh should remain but also call unprep to clear the frame buffer too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
greptile also warned about that.