[Lldb-commits] [PATCH] D78712: [lldb/Host] Improve error messages on unowned read files

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm not sure this new version is for the better :(.

Error and Expected objects have a contract where you're supposed to "consume" each object before you destroy or reassign it. The complex control flow in that function makes it very hard to see that this is really the case. I marked the cases where I believe you didn't do that, but I can't be sure I found all of them. For this kind of pattern to work there'd need to be more refactoring of the function in order to make the lifetime and state of the expected object more obvious. That's why I wasn't pushing for this direction in my previous comment...



================
Comment at: lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:96
 
-    if (FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec()))
-      error.Clear();
-    else {
-      const uint32_t permissions = FileSystem::Instance().GetPermissions(
-          resolved_module_spec.GetFileSpec());
-      if (permissions && (permissions & eFilePermissionsEveryoneR) == 0)
-        error.SetErrorStringWithFormat(
-            "executable '%s' is not readable",
-            resolved_module_spec.GetFileSpec().GetPath().c_str());
-      else
-        error.SetErrorStringWithFormat(
-            "unable to find executable for '%s'",
-            resolved_module_spec.GetFileSpec().GetPath().c_str());
+    resolved_module =
+        FileSystem::Instance().Open(resolved_module_spec.GetFileSpec(),
----------------
here you'll need to do a `consumeError(resolved_module.takeError())` before you can reuse the object.


================
Comment at: lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:124
+                                        exe_path);
+        return error;
+      }
----------------
error not consumed here


================
Comment at: lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:197
+      else
+        error.SetErrorStringWithFormatv(
+            "'{0}' doesn't contain any '{1}' platform architectures: {2}",
----------------
error not consumed here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78712/new/

https://reviews.llvm.org/D78712