Merge pull request #614 from jpcima/reloading

Avoid reloading invalid files in a loop
sfztools · Feb 1, 2021 · 872bc69 · 872bc69
2 parents 4ec9fd1 + 519bcb2
commit 872bc69
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 27 deletions.
diff --git a/src/sfizz/Synth.cpp b/src/sfizz/Synth.cpp
@@ -35,6 +35,9 @@
 
 namespace sfz {
 
+// unless set to permissive, the loader rejects sfz files with errors
+static constexpr bool loaderParsesPermissively = true;
+
 Synth::Synth()
 : impl_(new Impl) // NOLINT: (paul) I don't get why clang-tidy complains here
 {
@@ -252,7 +255,7 @@ void Synth::Impl::clear()
     masterOpcodes_.clear();
     groupOpcodes_.clear();
     unknownOpcodes_.clear();
-    modificationTime_ = fs::file_time_type::min();
+    modificationTime_ = absl::nullopt;
 
     // set default controllers
     // midistate is reset above
@@ -493,18 +496,22 @@ bool Synth::loadSfzFile(const fs::path& file)
     std::error_code ec;
     fs::path realFile = fs::canonical(file, ec);
 
-    impl.parser_.parseFile(ec ? file : realFile);
+    bool success = true;
+    Parser& parser = impl.parser_;
+    parser.parseFile(ec ? file : realFile);
+
     // permissive parsing for compatibility
-    if (false) {
-        if (impl.parser_.getErrorCount() > 0)
-            return false;
-    }
+    if (!loaderParsesPermissively)
+        success = parser.getErrorCount() == 0;
 
-    if (impl.regions_.empty())
+    success = success && !impl.regions_.empty();
+
+    if (!success) {
+        parser.clear();
         return false;
+    }
 
     impl.finalizeSfzLoad();
-
     return true;
 }
 
@@ -515,18 +522,22 @@ bool Synth::loadSfzString(const fs::path& path, absl::string_view text)
 
     impl.clear();
 
-    impl.parser_.parseString(path, text);
+    bool success = true;
+    Parser& parser = impl.parser_;
+    parser.parseString(path, text);
+
     // permissive parsing for compatibility
-    if (false) {
-        if (impl.parser_.getErrorCount() > 0)
-            return false;
-    }
+    if (!loaderParsesPermissively)
+        success = parser.getErrorCount() == 0;
+
+    success = success && !impl.regions_.empty();
 
-    if (impl.regions_.empty())
+    if (!success) {
+        parser.clear();
         return false;
+    }
 
     impl.finalizeSfzLoad();
-
     return true;
 }
 
@@ -1668,22 +1679,33 @@ void Synth::Impl::resetAllControllers(int delay) noexcept
     }
 }
 
-fs::file_time_type Synth::Impl::checkModificationTime()
+absl::optional<fs::file_time_type> Synth::Impl::checkModificationTime() const
 {
-    auto returnedTime = modificationTime_;
+    absl::optional<fs::file_time_type> resultTime;
     for (const auto& file : parser_.getIncludedFiles()) {
         std::error_code ec;
         const auto fileTime = fs::last_write_time(file, ec);
-        if (!ec && returnedTime < fileTime)
-            returnedTime = fileTime;
+        if (!ec) {
+            if (!resultTime || fileTime > *resultTime)
+                resultTime = fileTime;
+        }
     }
-    return returnedTime;
+    return resultTime;
 }
 
 bool Synth::shouldReloadFile()
 {
     Impl& impl = *impl_;
-    return (impl.checkModificationTime() > impl.modificationTime_);
+
+    absl::optional<fs::file_time_type> then = impl.modificationTime_;
+    if (!then) // file not loaded or failed
+        return false;
+
+    absl::optional<fs::file_time_type> now = impl.checkModificationTime();
+    if (!now) // file not currently existing
+        return false;
+
+    return *now > *then;
 }
 
 bool Synth::shouldReloadScala()

diff --git a/src/sfizz/SynthPrivate.h b/src/sfizz/SynthPrivate.h
@@ -110,9 +110,9 @@ struct Synth::Impl final: public Parser::Listener {
     /**
      * @brief Get the modification time of all included sfz files
      *
-     * @return fs::file_time_type
+     * @return absl::optional<fs::file_time_type>
      */
-    fs::file_time_type checkModificationTime();
+    absl::optional<fs::file_time_type> checkModificationTime() const;
 
     /**
      * @brief Check all regions and start voices for note on events
@@ -277,7 +277,7 @@ struct Synth::Impl final: public Parser::Listener {
     std::chrono::time_point<std::chrono::high_resolution_clock> lastGarbageCollection_;
 
     Parser parser_;
-    fs::file_time_type modificationTime_ { };
+    absl::optional<fs::file_time_type> modificationTime_ { };
 
     std::array<float, config::numCCs> defaultCCValues_;
     std::bitset<config::numCCs> currentUsedCCs_;

diff --git a/src/sfizz/parser/Parser.cpp b/src/sfizz/parser/Parser.cpp
@@ -19,7 +19,7 @@ Parser::~Parser()
 {
 }
 
-void Parser::reset()
+void Parser::clear()
 {
     _pathsIncluded.clear();
     _currentDefinitions = _externalDefinitions;
@@ -51,7 +51,7 @@ void Parser::parseString(const fs::path& path, absl::string_view sfzView)
 
 void Parser::parseVirtualFile(const fs::path& path, std::unique_ptr<Reader> reader)
 {
-    reset();
+    clear();
 
     if (_listener)
         _listener->onParseBegin();

diff --git a/src/sfizz/parser/Parser.h b/src/sfizz/parser/Parser.h
@@ -27,6 +27,8 @@ class Parser {
     Parser();
     ~Parser();
 
+    void clear();
+
     void addExternalDefinition(absl::string_view id, absl::string_view value);
     void clearExternalDefinitions();
 
@@ -71,7 +73,6 @@ class Parser {
     void processDirective();
     void processHeader();
     void processOpcode();
-    void reset();
 
     // errors and warnings
     void emitError(const SourceRange& range, const std::string& message);