[Lldb-commits] [lldb] [llvm] Modify the localCache API to require an explicit commit on CachedFile… (PR #115331)

[via lldb-commits]

================
@@ -131,11 +138,22 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
         });
 
         if (E)
-          report_fatal_error(Twine("Failed to rename temporary file ") +
-                             TempFile.TmpName + " to " + ObjectPathName + ": " +
-                             toString(std::move(E)) + "\n");
+          return E;
 
         AddBuffer(Task, ModuleName, std::move(*MBOrErr));
+        return Error::success();
+      }
+
+      ~CacheStream() {
+        // In Debug builds, try to track down places where commit() was not
+        // called before destruction.
+        assert(Committed);
----------------
anjenner wrote:

The advantage of explicit commit is that it allows proper error handling rather than aborting the entire process if the cache write fails - usually an undesirable behavior. My aim here was to quietly fall back to the previous behavior in if commit() was not called in release mode but help developers track down such places in debug mode (which worked perfectly - I found just such a case that way). I was thinking of leaving it there to help people who have code that uses localCache in branches that have not yet landed in mainline LLVM. However, I agree that it would be annoying to switch from release to debug to track down an unrelated problem and have to fix a missing commit() at that point. I think on balance explicitly failing here is the better plan - I'll modify my patch to do this but I could probably be talked into allowing the commit() to be optional if anyone has strong feelings about this.

https://github.com/llvm/llvm-project/pull/115331