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

[Kyungwoo Lee via lldb-commits]

@@ -88,9 +89,10 @@ Expected llvm::localCache(const Twine 
&CacheNameRef,
 AddBuffer(std::move(AddBuffer)), TempFile(std::move(TempFile)),
 ModuleName(ModuleName), Task(Task) {}
 
-  ~CacheStream() {
-// TODO: Manually commit rather than using non-trivial destructor,
-// allowing to replace report_fatal_errors with a return Error.
+  Error commit() override {
+if (Committed)
+  return Error::success();

kyulee-com wrote:

Intuitively, this doesn't seem correct. Shouldn't we fail if we attempt to 
commit more than once, rather than just returning a success?
Additionally, I'm curious about what happens if we use this CacheStream after a 
commit has been called. I suspect it will fail, but do we provide a proper 
error message in this case? It would be ideal to have a test case to cover 
these scenarios.

https://github.com/llvm/llvm-project/pull/115331
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Kyungwoo Lee via lldb-commits]

@@ -131,11 +138,22 @@ Expected 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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits