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

[Kyungwoo Lee via llvm-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);
----------------
kyulee-com wrote:

While I understand the intent here, it's unusual to have different behavior in debug and release modes. The release mode operation, where commit() is essentially optional because it's handled by the destructor, seems reasonable to me. For consistency, I would prefer to remove this assert, although you can use it for testing purposes. Are you aiming to use commit() optionally to make error messages more explicit?

Alternatively, if we require commit() to be used instead of relying on the destructor, I would suggest explicitly failing or emitting a warning if it hasn't been committed beforehand.

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