Conversation
…tale files from a previous run if this one fails)
Commands/CDirectiveFile.cpp
Outdated
| lineEnd = std::min(line.find_first_of(";\r\x1A"),lineEnd); | ||
| if (lineEnd != std::string::npos && lineEnd > 0) | ||
| { | ||
| lineEnd = std::max(line.find_last_not_of(" \t",lineEnd-1)+1,(size_t)0); |
There was a problem hiding this comment.
Is std::max here needed? If you compare against std::string::npos in the if below you should be able to just take the result of find_last_not_of.
This block could probably also be extracted into a trim utility function (e.g. in a local namespace).
Commands/CDirectiveFile.h
Outdated
| { | ||
| public: | ||
| CDirectiveSymImport(const fs::path& fileName); | ||
| ~CDirectiveSymImport() { }; |
There was a problem hiding this comment.
You can omit the destructor if it doesn't do any custom logic. If you want to make that explicit, override = default.
Commands/CDirectiveFile.cpp
Outdated
| return; | ||
| } | ||
|
|
||
| fs::ifstream file(this->fileName,fs::ifstream::in|fs::ifstream::binary); |
There was a problem hiding this comment.
Why binary? Also, why not use TextFile? That also comes with a utility function to read all lines, which would make the loop below simpler.
|
|
||
| // Create label for this symbol, if it's not a directive and it would be a global label | ||
| const Identifier identifier = Identifier(name); | ||
| if (name.find('.') != 0 && Global.symbolTable.isGlobalSymbol(identifier)) |
There was a problem hiding this comment.
Symbols can contain periods (see SymbolTable::isValidSymbolName) - but they can't contain colons, which .sym uses inside of its directives. So perhaps:
if (name.find(':') == std::string::npos && Global.symbolTable.isValidSymbolName(identifier) && Global.symbolTable.isGlobalSymbol(identifier))
This would make the nullptr check below redundant, however, so the check against isValidSymbolName could be used there instead too.
There was a problem hiding this comment.
I think we should still prohibit symbols starting with a period. Not all symfile directives use colons; e.g. .arm, .thumb and .pool do not, and I don't think it makes sense to create a label named .pool. It's also easier to understand.
| std::string name = line.substr(startOfName); | ||
|
|
||
| // Create label for this symbol, if it's not a directive and it would be a global label | ||
| const Identifier identifier = Identifier(name); |
There was a problem hiding this comment.
This works for no$gba .sym files (or -sym output), but not for files generated by -sym2 (if .func was used, at least). Anything after a comma should also be removed from the identifier.
2 participants
This adds the
.importsymdirective which lets you import a pre-existing symfile. This is pretty useful when you have a base symfile for a game and you want to merge all your new labels into it.Currently this enforces No$gba semantics where each address must be exactly 8 characters. I'm not sure if
sym2format symfiles allow for different lengths; if so, the restriction could probably be removed.Also, this doesn't remove or change any data in the output symfile that's overwritten by armips. E.g., you have a memory region that's defined as a data pool and you write a function in there instead, the original data pool definition will be copied to the output symfile as-is (in addition to any new labels you placed in that region). I don't think this is a big problem, though.