readCDefines() - don't crash on invalid expressions, add better debugging info

include/core/parseutil.h

@@ -2,6 +2,7 @@
 #define PARSEUTIL_H
 
 #include "heallocation.h"
+#include "log.h"
 
 #include <QString>
 #include <QList>
@@ -10,6 +11,30 @@
 enum TokenType {
     Number,
     Operator,
+    Error,
+};
+
+class DebugInfo {
+public:
+    DebugInfo(QString file, QStringList lines) {
+        this->file = file;
+        this->lines = lines;
+    };
+    QString file;
+    int     line;
+    bool    err;
+    QStringList lines;
+    void error(QString expression, QString token) {
+        int lineNo = 0;
+        for (QString line_ : lines) {
+            lineNo++;
+            if (line_.contains(expression)) {
+                this->line = lineNo;
+                break;
+            }
+        }
+        logError(QString("%1:%2: unknown identifier found in expression: '%3'.").arg(file).arg(line).arg(token));
+    }
 };
 
 class Token {
@@ -22,6 +47,8 @@ public:
             this->operatorPrecedence = -1;
         } else if (type == "operator") {
             this->operatorPrecedence = precedenceMap[value];
+        } else if (type == "error") {
+            this->type = TokenType::Error;
         }
     }
     static QMap<QString, int> precedenceMap;
@@ -33,10 +60,11 @@ public:
 class ParseUtil
 {
 public:
-    ParseUtil();
+    ParseUtil(QString, QString);
     void strip_comment(QString*);
     QList<QStringList>* parseAsm(QString);
     int evaluateDefine(QString, QMap<QString, int>*);
+    DebugInfo *debug;
 private:
     QList<Token> tokenizeExpression(QString expression, QMap<QString, int>* knownIdentifiers);
     QList<Token> generatePostfix(QList<Token> tokens);

include/project.h

@@ -138,7 +138,7 @@ public:
 
     QStringList readCArray(QString text, QString label);
     QString readCIncbin(QString text, QString label);
-    QMap<QString, int> readCDefines(QString text, QStringList prefixes);
+    QMap<QString, int> readCDefines(QString filename, QStringList prefixes);
     QMap<QString, QString> readNamedIndexCArray(QString text, QString label);
 
     static int getNumTilesPrimary();

src/core/parseutil.cpp

@@ -4,8 +4,10 @@
 #include <QRegularExpression>
 #include <QStack>
 
-ParseUtil::ParseUtil()
+ParseUtil::ParseUtil(QString filename, QString text)
 {
+    QStringList lines = text.split(QRegularExpression("[\r\n]"));
+    debug = new DebugInfo(filename, lines);
 }
 
 void ParseUtil::strip_comment(QString *line) {
@@ -91,7 +93,8 @@ QList<Token> ParseUtil::tokenizeExpression(QString expression, QMap<QString, int
                         token = actualToken;
                         tokenType = "decimal";
                     } else {
-                        logError(QString("Unknown identifier found in expression: '%1'").arg(token));
+                        tokenType = "error";
+                        debug->error(expression, token);
                     }
                 }
 
@@ -190,9 +193,9 @@ int ParseUtil::evaluatePostfix(QList<Token> postfix) {
                 logError(QString("Unsupported postfix operator: '%1'").arg(token.value));
             }
             stack.push(Token(QString("%1").arg(result), "decimal"));
-        } else {
+        } else if (token.type != TokenType::Error) {
             stack.push(token);
-        }
+        } // else ignore errored tokens, we have already warned the user.
     }
 
     return stack.pop().value.toInt(nullptr, 0);

src/log.cpp

@@ -2,6 +2,21 @@
 #include <QDateTime>
 #include <QDir>
 #include <QStandardPaths>
+#include <QSysInfo>
+
+// Enabling this does not seem to be simple to color console output
+// on Windows for all CLIs without external libraries or extreme bloat.
+#ifdef Q_OS_WIN
+    #define ERROR_COLOR   ""
+    #define WARNING_COLOR ""
+    #define INFO_COLOR    ""
+    #define CLEAR_COLOR   ""
+#else
+    #define ERROR_COLOR   "\033[31;1m"
+    #define WARNING_COLOR "\033[1;33m"
+    #define INFO_COLOR    "\033[32m"
+    #define CLEAR_COLOR   "\033[0m"
+#endif
 
 void logInfo(QString message) {
     log(message, LogType::LOG_INFO);
@@ -15,6 +30,23 @@ void logError(QString message) {
     log(message, LogType::LOG_ERROR);
 }
 
+QString colorizeMessage(QString message, LogType type) {
+    QString colorized = message;
+    switch (type)
+    {
+    case LogType::LOG_INFO:
+        colorized = colorized.replace("INFO", INFO_COLOR "INFO" CLEAR_COLOR);
+        break;
+    case LogType::LOG_WARN:
+        colorized = colorized.replace("WARN", WARNING_COLOR "WARN" CLEAR_COLOR);
+        break;
+    case LogType::LOG_ERROR:
+        colorized = colorized.replace("ERROR", ERROR_COLOR "ERROR" CLEAR_COLOR);
+        break;
+    }
+    return colorized;
+}
+
 void log(QString message, LogType type) {
     QString now = QDateTime::currentDateTime().toString("yyyy-MM-dd HH:mm:ss");
     QString typeString = "";
@@ -40,7 +72,7 @@ void log(QString message, LogType type) {
 
     QString logPath = dir.absoluteFilePath("porymap.log");
 
-    qDebug() << message;
+    qDebug().noquote() << colorizeMessage(message, type);
     QFile outFile(logPath);
     outFile.open(QIODevice::WriteOnly | QIODevice::Append);
     QTextStream ts(&outFile);

src/project.cpp

@@ -729,9 +729,9 @@ void Project::loadMapTilesets(Map* map) {
 }
 
 Tileset* Project::loadTileset(QString label, Tileset *tileset) {
-    ParseUtil *parser = new ParseUtil;
+    QString filename = "data/tilesets/headers.inc";
-
+    QString headers_text = readTextFile(root + "/" + filename);
-    QString headers_text = readTextFile(root + "/data/tilesets/headers.inc");
+    ParseUtil *parser = new ParseUtil(filename, headers_text);
     if (headers_text.isNull()) {
         return nullptr;
     }
@@ -1031,7 +1031,6 @@ void Project::saveAllDataStructures() {
 }
 
 void Project::loadTilesetAssets(Tileset* tileset) {
-    ParseUtil* parser = new ParseUtil;
     QString category = (tileset->is_secondary == "TRUE") ? "secondary" : "primary";
     if (tileset->name.isNull()) {
         return;
@@ -1039,7 +1038,9 @@ void Project::loadTilesetAssets(Tileset* tileset) {
     QString tilesetName = tileset->name;
     QString dir_path = root + "/data/tilesets/" + category + "/" + tilesetName.replace("gTileset_", "").toLower();
 
-    QString graphics_text = readTextFile(root + "/data/tilesets/graphics.inc");
+    QString gfx_filename = "data/tilesets/graphics.inc";
+    QString graphics_text = readTextFile(root + "/" + gfx_filename);
+    ParseUtil* parser = new ParseUtil(gfx_filename, graphics_text);
     QList<QStringList> *graphics = parser->parseAsm(graphics_text);
     QStringList *tiles_values = getLabelValues(graphics, tileset->tiles_label);
     QStringList *palettes_values = getLabelValues(graphics, tileset->palettes_label);
@@ -1374,11 +1375,6 @@ QString Project::getNewMapName() {
     return newMapName;
 }
 
-QList<QStringList>* Project::parseAsm(QString text) {
-    ParseUtil *parser = new ParseUtil;
-    return parser->parseAsm(text);
-}
-
 QStringList Project::getVisibilities() {
     // TODO
     QStringList names;
@@ -1418,8 +1414,8 @@ QMap<QString, QStringList> Project::getTilesetLabels() {
 
         if (secondaryTilesetValue != "1" && secondaryTilesetValue != "TRUE" 
          && secondaryTilesetValue != "0" && secondaryTilesetValue != "FALSE") {
-            logWarn(QString("Unexpected secondary tileset flag found."
+            logWarn(QString("Unexpected secondary tileset flag found in %1. Expected 'TRUE', 'FALSE', '1', or '0', but found '%2'")
-                "Expected \"TRUE\", \"FALSE\", \"0\", or \"1\", but found: '%1'").arg(secondaryTilesetValue));
+                    .arg(tilesetLabel).arg(secondaryTilesetValue));
             continue;
         }
 
@@ -1434,77 +1430,73 @@ QMap<QString, QStringList> Project::getTilesetLabels() {
 }
 
 void Project::readTilesetProperties() {
-    QStringList defines;
+    QString filename = "include/fieldmap.h";
-    QString text = readTextFile(root + "/include/fieldmap.h");
+        
-    if (!text.isNull()) {
+    bool error = false;
-        bool error = false;
+    QStringList definePrefixes;
-        QStringList definePrefixes;
+    definePrefixes << "NUM_";
-        definePrefixes << "NUM_";
+    QMap<QString, int> defines = readCDefines(filename, definePrefixes);
-        QMap<QString, int> defines = readCDefines(text, definePrefixes);
-        auto it = defines.find("NUM_TILES_IN_PRIMARY");
-        if (it != defines.end()) {
-            Project::num_tiles_primary = it.value();
-        }
-        else {
-            error = true;
-        }
-        it = defines.find("NUM_TILES_TOTAL");
-        if (it != defines.end()) {
-            Project::num_tiles_total = it.value();
-        }
-        else {
-            error = true;
-        }
-        it = defines.find("NUM_METATILES_IN_PRIMARY");
-        if (it != defines.end()) {
-            Project::num_metatiles_primary = it.value();
-        }
-        else {
-            error = true;
-        }
-        it = defines.find("NUM_METATILES_TOTAL");
-        if (it != defines.end()) {
-            Project::num_metatiles_total = it.value();
-        }
-        else {
-            error = true;
-        }
-        it = defines.find("NUM_PALS_IN_PRIMARY");
-        if (it != defines.end()) {
-            Project::num_pals_primary = it.value();
-        }
-        else {
-            error = true;
-        }
-        it = defines.find("NUM_PALS_TOTAL");
-        if (it != defines.end()) {
-            Project::num_pals_total = it.value();
-        }
-        else {
-            error = true;
-        }
 
-        if (error)
+    auto it = defines.find("NUM_TILES_IN_PRIMARY");
-        {
+    if (it != defines.end()) {
-            logError("Some global tileset values could not be loaded. Using default values instead.");
+        Project::num_tiles_primary = it.value();
-        }
+    }
+    else {
+        error = true;
+    }
+    it = defines.find("NUM_TILES_TOTAL");
+    if (it != defines.end()) {
+        Project::num_tiles_total = it.value();
+    }
+    else {
+        error = true;
+    }
+    it = defines.find("NUM_METATILES_IN_PRIMARY");
+    if (it != defines.end()) {
+        Project::num_metatiles_primary = it.value();
+    }
+    else {
+        error = true;
+    }
+    it = defines.find("NUM_METATILES_TOTAL");
+    if (it != defines.end()) {
+        Project::num_metatiles_total = it.value();
+    }
+    else {
+        error = true;
+    }
+    it = defines.find("NUM_PALS_IN_PRIMARY");
+    if (it != defines.end()) {
+        Project::num_pals_primary = it.value();
+    }
+    else {
+        error = true;
+    }
+    it = defines.find("NUM_PALS_TOTAL");
+    if (it != defines.end()) {
+        Project::num_pals_total = it.value();
+    }
+    else {
+        error = true;
+    }
+
+    if (error)
+    {
+        logError("Some global tileset values could not be loaded. Using default values instead.");
     }
 }
 
 void Project::readRegionMapSections() {
-    QString filepath = root + "/include/constants/region_map_sections.h";
+    QString filename = "include/constants/region_map_sections.h";
     this->mapSectionNameToValue.clear();
     this->mapSectionValueToName.clear();
-    QString text = readTextFile(filepath);
+
-    if (!text.isNull()) {
+    QStringList prefixes = (QStringList() << "MAPSEC_");
-        QStringList prefixes = (QStringList() << "MAPSEC_");
+    this->mapSectionNameToValue = readCDefines(filename, prefixes);
-        this->mapSectionNameToValue = readCDefines(text, prefixes);
+    for (QString defineName : this->mapSectionNameToValue.keys()) {
-        for (QString defineName : this->mapSectionNameToValue.keys()) {
+        this->mapSectionValueToName.insert(this->mapSectionNameToValue[defineName], defineName);
-            this->mapSectionValueToName.insert(this->mapSectionNameToValue[defineName], defineName);
-        }
-    } else {
-        logError(QString("Failed to read C defines file: '%1'").arg(filepath));
     }
+
 }
 
 void Project::readHealLocations() {
@@ -1526,27 +1518,27 @@ void Project::readHealLocations() {
 }
 
 void Project::readItemNames() {
-    QString filepath = root + "/include/constants/items.h";
+    QString filename = "include/constants/items.h";
     QStringList prefixes = (QStringList() << "ITEM_");
-    readCDefinesSorted(filepath, prefixes, itemNames);
+    readCDefinesSorted(filename, prefixes, itemNames);
 }
 
 void Project::readFlagNames() {
-    QString filepath = root + "/include/constants/flags.h";
+    QString filename = "include/constants/flags.h";
     QStringList prefixes = (QStringList() << "FLAG_");
-    readCDefinesSorted(filepath, prefixes, flagNames);
+    readCDefinesSorted(filename, prefixes, flagNames);
 }
 
 void Project::readVarNames() {
-    QString filepath = root + "/include/constants/vars.h";
+    QString filename = "include/constants/vars.h";
     QStringList prefixes = (QStringList() << "VAR_");
-    readCDefinesSorted(filepath, prefixes, varNames);
+    readCDefinesSorted(filename, prefixes, varNames);
 }
 
 void Project::readMovementTypes() {
-    QString filepath = root + "/include/constants/event_object_movement_constants.h";
+    QString filename = "include/constants/event_object_movement_constants.h";
     QStringList prefixes = (QStringList() << "MOVEMENT_TYPE_");
-    readCDefinesSorted(filepath, prefixes, movementTypes);
+    readCDefinesSorted(filename, prefixes, movementTypes);
 }
 
 void Project::readInitialFacingDirections() {
@@ -1555,94 +1547,89 @@ void Project::readInitialFacingDirections() {
 }
 
 void Project::readMapTypes() {
-    QString filepath = root + "/include/constants/map_types.h";
+    QString filename = "include/constants/map_types.h";
     QStringList prefixes = (QStringList() << "MAP_TYPE_");
-    readCDefinesSorted(filepath, prefixes, mapTypes);
+    readCDefinesSorted(filename, prefixes, mapTypes);
 }
 
 void Project::readMapBattleScenes() {
-    QString filepath = root + "/include/constants/map_types.h";
+    QString filename = "include/constants/map_types.h";
     QStringList prefixes = (QStringList() << "MAP_BATTLE_SCENE_");
-    readCDefinesSorted(filepath, prefixes, mapBattleScenes);
+    readCDefinesSorted(filename, prefixes, mapBattleScenes);
 }
 
 void Project::readWeatherNames() {
-    QString filepath = root + "/include/constants/weather.h";
+    QString filename = "include/constants/weather.h";
     QStringList prefixes = (QStringList() << "WEATHER_");
-    readCDefinesSorted(filepath, prefixes, weatherNames);
+    readCDefinesSorted(filename, prefixes, weatherNames);
 }
 
 void Project::readCoordEventWeatherNames() {
-    QString filepath = root + "/include/constants/weather.h";
+    QString filename = "include/constants/weather.h";
     QStringList prefixes = (QStringList() << "COORD_EVENT_WEATHER_");
-    readCDefinesSorted(filepath, prefixes, coordEventWeatherNames);
+    readCDefinesSorted(filename, prefixes, coordEventWeatherNames);
 }
 
 void Project::readSecretBaseIds() {
-    QString filepath = root + "/include/constants/secret_bases.h";
+    QString filename = "include/constants/secret_bases.h";
     QStringList prefixes = (QStringList() << "SECRET_BASE_[A-Za-z0-9_]*_[0-9]+");
-    readCDefinesSorted(filepath, prefixes, secretBaseIds);
+    readCDefinesSorted(filename, prefixes, secretBaseIds);
 }
 
 void Project::readBgEventFacingDirections() {
-    QString filepath = root + "/include/constants/bg_event_constants.h";
+    QString filename = "include/constants/bg_event_constants.h";
     QStringList prefixes = (QStringList() << "BG_EVENT_PLAYER_FACING_");
-    readCDefinesSorted(filepath, prefixes, bgEventFacingDirections);
+    readCDefinesSorted(filename, prefixes, bgEventFacingDirections);
 }
 
 void Project::readMetatileBehaviors() {
     this->metatileBehaviorMap.clear();
     this->metatileBehaviorMapInverse.clear();
-    QString filepath = root + "/include/constants/metatile_behaviors.h";
+    QString filename = "include/constants/metatile_behaviors.h";
-    QString text = readTextFile(filepath);
+
-    if (!text.isNull()) {
+    QStringList prefixes = (QStringList() << "MB_");
-        QStringList prefixes = (QStringList() << "MB_");
+    this->metatileBehaviorMap = readCDefines(filename, prefixes);
-        this->metatileBehaviorMap = readCDefines(text, prefixes);
+    for (QString defineName : this->metatileBehaviorMap.keys()) {
-        for (QString defineName : this->metatileBehaviorMap.keys()) {
+        this->metatileBehaviorMapInverse.insert(this->metatileBehaviorMap[defineName], defineName);
-            this->metatileBehaviorMapInverse.insert(this->metatileBehaviorMap[defineName], defineName);
-        }
-    } else {
-        logError(QString("Failed to read C defines file: '%1'").arg(filepath));
     }
 }
 
-void Project::readCDefinesSorted(QString filepath, QStringList prefixes, QStringList* definesToSet) {
+void Project::readCDefinesSorted(QString filename, QStringList prefixes, QStringList* definesToSet) {
-    QString text = readTextFile(filepath);
+    QString filepath = root + "/" + filename;
-    if (!text.isNull()) {
+    
-        QMap<QString, int> defines = readCDefines(text, prefixes);
+    QMap<QString, int> defines = readCDefines(filename, prefixes);
 
-        // The defines should to be sorted by their underlying value, not alphabetically.
+    // The defines should to be sorted by their underlying value, not alphabetically.
-        // Reverse the map and read out the resulting keys in order.
+    // Reverse the map and read out the resulting keys in order.
-        QMultiMap<int, QString> definesInverse;
+    QMultiMap<int, QString> definesInverse;
-        for (QString defineName : defines.keys()) {
+    for (QString defineName : defines.keys()) {
-            definesInverse.insert(defines[defineName], defineName);
+        definesInverse.insert(defines[defineName], defineName);
-        }
-        *definesToSet = definesInverse.values();
-    } else {
-        logError(QString("Failed to read C defines file: '%1'").arg(filepath));
     }
+    *definesToSet = definesInverse.values();
 }
 
 QStringList Project::getSongNames() {
     QStringList names;
-    QString text = readTextFile(root + "/include/constants/songs.h");
+    QString filename = "include/constants/songs.h";
-    if (!text.isNull()) {
+
-        QStringList songDefinePrefixes;
+    QStringList songDefinePrefixes;
-        songDefinePrefixes << "SE_" << "MUS_";
+    songDefinePrefixes << "SE_" << "MUS_";
-        QMap<QString, int> songDefines = readCDefines(text, songDefinePrefixes);
+    QMap<QString, int> songDefines = readCDefines(filename, songDefinePrefixes);
-        names = songDefines.keys();
+    names = songDefines.keys();
-    }
+
     return names;
 }
 
 QMap<QString, int> Project::getEventObjGfxConstants() {
     QMap<QString, int> constants;
-    QString text = readTextFile(root + "/include/constants/event_objects.h");
+    QString filename = "include/constants/event_objects.h";
-    if (!text.isNull()) {
+    QString filepath = root + "/" + filename;
-        QStringList eventObjGfxPrefixes;
+    QString text = readTextFile(filepath);
-        eventObjGfxPrefixes << "EVENT_OBJ_GFX_";
+
-        constants = readCDefines(text, eventObjGfxPrefixes);
+    QStringList eventObjGfxPrefixes;
-    }
+    eventObjGfxPrefixes << "EVENT_OBJ_GFX_";
+    constants = readCDefines(filename, eventObjGfxPrefixes);
+
     return constants;
 }
 
@@ -1819,11 +1806,26 @@ QString Project::readCIncbin(QString text, QString label) {
     return path;
 }
 
-QMap<QString, int> Project::readCDefines(QString text, QStringList prefixes) {
+QMap<QString, int> Project::readCDefines(QString filename, QStringList prefixes) {
-    ParseUtil parser;
-    text.replace(QRegularExpression("(//.*)|(\\/+\\*+[^*]*\\*+\\/+)"), "");
     QMap<QString, int> allDefines;
     QMap<QString, int> filteredDefines;
+
+    if (filename.isEmpty()) {
+        return filteredDefines;
+    }
+
+    QString filepath = root + "/" + filename;
+    QString text = readTextFile(filepath);
+
+    if (text.isNull()) {
+        logError(QString("Failed to read C defines file: '%1'").arg(filepath));
+        return filteredDefines;
+    }
+
+    ParseUtil parser(filename, text);
+
+    text.replace(QRegularExpression("(//.*)|(\\/+\\*+[^*]*\\*+\\/+)"), "");
+
     QRegularExpression re("#define\\s+(?<defineName>\\w+)[^\\S\\n]+(?<defineValue>.+)");
     QRegularExpressionMatchIterator iter = re.globalMatch(text);
     while (iter.hasNext()) {