Refactors and comments

include/errors_warnings.h

@@ -178,7 +178,7 @@ void fatalerror_invalidIdInCsv(const ErrorsAndWarnings &err, const SourcePaths &
                                std::string filePath, std::string id, std::size_t line);
 
 void fatalerror_invalidBehaviorValue(const ErrorsAndWarnings &err, const SourcePaths &srcs, CompilerMode mode,
-                                     std::string filePath, std::string behavior, std::string value, std::size_t line);
+                                     std::string behavior, std::string value, std::size_t line);
 
 // Compilation warnings (due to possible mistakes in user input), compilation can continue
 void warn_colorPrecisionLoss(ErrorsAndWarnings &err, const RGBATile &tile, std::size_t row, std::size_t col,

include/importer.h

@@ -10,8 +10,10 @@
 #include "types.h"
 
 /**
- * Utility functions for reading input from various file types. Provides utilities for reading a layered or raw
- * tilesheet.
+ * Utility functions for building core Porytiles types from sanitized input types and data structures. The importer
+ * functions don't handle any file checking or input collation. They expect to receive data as ready-to-use ifstreams,
+ * png::images, and other data structures. The driver is responsible for preparing these types and data structures from
+ * the raw input files.
  */
 
 namespace porytiles {
@@ -32,7 +34,7 @@ void importAnimTiles(PtContext &ctx, const std::vector<std::vector<AnimationPng<
                      DecompiledTileset &tiles);
 
 std::pair<std::unordered_map<std::string, std::uint8_t>, std::unordered_map<std::uint8_t, std::string>>
-importMetatileBehaviorMaps(PtContext &ctx, const std::string &filePath);
+importMetatileBehaviorMaps(PtContext &ctx, std::ifstream &behaviorFile);
 
 std::unordered_map<std::size_t, Attributes>
 importAttributesFromCsv(PtContext &ctx, const std::unordered_map<std::string, std::uint8_t> &behaviorMap,

src/driver.cpp

@@ -298,7 +298,12 @@ static void driveCompile(PtContext &ctx)
   std::unordered_map<std::string, std::uint8_t> behaviorMap{};
   std::unordered_map<std::uint8_t, std::string> behaviorReverseMap{};
   if (std::filesystem::exists(ctx.srcPaths.primaryMetatileBehaviors())) {
-    auto [map, reverse] = importMetatileBehaviorMaps(ctx, ctx.srcPaths.primaryMetatileBehaviors());
+    std::ifstream behaviorFile{ctx.srcPaths.primaryMetatileBehaviors()};
+    if (behaviorFile.fail()) {
+      fatalerror(ctx.err, ctx.srcPaths, ctx.compilerConfig.mode,
+                 fmt::format("{}: could not open for reading", ctx.srcPaths.primaryMetatileBehaviors().string()));
+    }
+    auto [map, reverse] = importMetatileBehaviorMaps(ctx, behaviorFile);
     behaviorMap = map;
     behaviorReverseMap = reverse;
   }

src/errors_warnings.cpp

@@ -415,16 +415,15 @@ void fatalerror_invalidIdInCsv(const ErrorsAndWarnings &err, const SourcePaths &
 }
 
 void fatalerror_invalidBehaviorValue(const ErrorsAndWarnings &err, const SourcePaths &srcs, CompilerMode mode,
-                                     std::string filePath, std::string behavior, std::string value, std::size_t line)
+                                     std::string behavior, std::string value, std::size_t line)
 {
   if (err.printErrors) {
-    pt_fatal_err("{}: invalid value '{}' for behavior '{}' defined at line {}", filePath,
-                 fmt::styled(value, fmt::emphasis::bold), fmt::styled(behavior, fmt::emphasis::bold), line);
+    pt_fatal_err("invalid value '{}' for behavior '{}' defined at line {}", fmt::styled(value, fmt::emphasis::bold),
+                 fmt::styled(behavior, fmt::emphasis::bold), line);
     pt_note("behavior must be an integral value (both decimal and hexidecimal notations are permitted)",
             fmt::styled("id", fmt::emphasis::bold));
   }
-  die_compilationTerminated(err, srcs.modeBasedSrcPath(mode),
-                            fmt::format("{}: invalid behavior value {}", filePath, value));
+  die_compilationTerminated(err, srcs.modeBasedSrcPath(mode), fmt::format("invalid behavior value {}", value));
 }
 
 static void printWarning(ErrorsAndWarnings &err, WarningMode warningMode, const std::string_view &warningName,
@@ -971,15 +970,15 @@ TEST_CASE("fatalerror_invalidBehaviorValue should trigger when the metatile beha
 
   SUBCASE("Invalid integer format 1")
   {
-    CHECK_THROWS_WITH_AS(porytiles::importMetatileBehaviorMaps(ctx, "res/tests/metatile_behaviors_invalid_1.h"),
-                         "res/tests/metatile_behaviors_invalid_1.h: invalid behavior value foo",
+    std::ifstream behaviorFile{"res/tests/metatile_behaviors_invalid_1.h"};
+    CHECK_THROWS_WITH_AS(porytiles::importMetatileBehaviorMaps(ctx, behaviorFile), "invalid behavior value foo",
                          porytiles::PtException);
   }
 
   SUBCASE("Invalid integer format 2")
   {
-    CHECK_THROWS_WITH_AS(porytiles::importMetatileBehaviorMaps(ctx, "res/tests/metatile_behaviors_invalid_2.h"),
-                         "res/tests/metatile_behaviors_invalid_2.h: invalid behavior value 6bar",
+    std::ifstream behaviorFile{"res/tests/metatile_behaviors_invalid_2.h"};
+    CHECK_THROWS_WITH_AS(porytiles::importMetatileBehaviorMaps(ctx, behaviorFile), "invalid behavior value 6bar",
                          porytiles::PtException);
   }
 }

src/importer.cpp

@@ -410,15 +410,10 @@ void importAnimTiles(PtContext &ctx, const std::vector<std::vector<AnimationPng<
 }
 
 std::pair<std::unordered_map<std::string, std::uint8_t>, std::unordered_map<std::uint8_t, std::string>>
-importMetatileBehaviorMaps(PtContext &ctx, const std::string &filePath)
+importMetatileBehaviorMaps(PtContext &ctx, std::ifstream &behaviorFile)
 {
   std::unordered_map<std::string, std::uint8_t> behaviorMap{};
   std::unordered_map<std::uint8_t, std::string> behaviorReverseMap{};
-  std::ifstream behaviorFile{filePath};
-
-  if (behaviorFile.fail()) {
-    fatalerror(ctx.err, ctx.srcPaths, ctx.compilerConfig.mode, fmt::format("{}: could not open for reading", filePath));
-  }
 
   std::string line;
   std::size_t processedUpToLine = 1;
@@ -444,7 +439,7 @@ importMetatileBehaviorMaps(PtContext &ctx, const std::string &filePath)
       }
       catch (const std::exception &e) {
         behaviorFile.close();
-        fatalerror_invalidBehaviorValue(ctx.err, ctx.srcPaths, ctx.compilerConfig.mode, filePath, behaviorName,
+        fatalerror_invalidBehaviorValue(ctx.err, ctx.srcPaths, ctx.compilerConfig.mode, behaviorName,
                                         behaviorValueString, processedUpToLine);
         // here so compiler won't complain
         behaviorVal = 0;
@@ -687,7 +682,10 @@ static std::vector<Assignment> importCompiledMetatilesAndAttrs(PtContext &ctx, s
 
 CompiledTileset importCompiledTileset(PtContext &ctx, const std::filesystem::path &tilesetPath)
 {
-  // TODO : who should handle checks for file existence/validity? importer or driver?
+  /*
+   * TODO : who should handle checks for file existence/validity? importer or driver? I think driver should, so we can
+   * refactor this function later so that it receives everything it needs as ifstreams or png::images
+   */
   CompiledTileset tileset{};
   std::ifstream metatiles{tilesetPath / "metatiles.bin", std::ios::binary};
   std::ifstream attributes{tilesetPath / "metatile_attributes.bin", std::ios::binary};
@@ -1126,7 +1124,8 @@ TEST_CASE("importMetatileBehaviorMaps should parse metatile behaviors as expecte
   ctx.compilerConfig.mode = porytiles::CompilerMode::PRIMARY;
   ctx.err.printErrors = false;
 
-  auto [behaviorMap, behaviorReverseMap] = porytiles::importMetatileBehaviorMaps(ctx, "res/tests/metatile_behaviors.h");
+  std::ifstream behaviorFile{"res/tests/metatile_behaviors.h"};
+  auto [behaviorMap, behaviorReverseMap] = porytiles::importMetatileBehaviorMaps(ctx, behaviorFile);
 
   CHECK(!behaviorMap.contains("MB_INVALID"));
   CHECK(behaviorMap.at("MB_NORMAL") == 0x00);

src/main.cpp

@@ -26,17 +26,22 @@ try {
   return 0;
 }
 catch (const porytiles::PtException &e) {
-  // Catch PtException here, these are errors that can reasonably be expected due to bad input, bad files, etc
+  /*
+   * Catch PtException here. This exception is used by the error system to indicate an error it correctly handled and
+   * reported to the user. These errors are typically due to invalid user input. So we can just return 1 here to
+   * indicate a bad exit.
+   */
   return 1;
 }
 catch (const std::exception &e) {
-  // New C++23 features may allow a stacktrace here: https://github.com/TylerGlaiel/Crashlogs
-  // Or do something like this: https://stackoverflow.com/questions/691719/c-display-stack-trace-on-exception
-
   /*
-   * Any other exception type indicates an internal compiler error -- dump a helpful message so the user can file an
-   * issue in the repo.
+   * Any other exception type indicates an internal compiler error, i.e. an error we did not explicitly handle or
+   * anticipate from library code, or an error we explicitly threw due to an unrecoverable assert failure. This usually
+   * indicates a bug in the compiler. Just dump a helpful message so the user can file an issue on GitHub.
    */
+
+  // New C++23 features may allow a stacktrace here: https://github.com/TylerGlaiel/Crashlogs
+  // Or do something like this: https://stackoverflow.com/questions/691719/c-display-stack-trace-on-exception
   porytiles::pt_println(stderr, "{}: {} {}", porytiles::PROGRAM_NAME,
                         fmt::styled("internal compiler error:", fmt::emphasis::bold | fg(fmt::terminal_color::yellow)),
                         e.what());