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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

I also prefer this approach, seems less fragile from a testing perspective and reduces the different code paths.



================
Comment at: lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:102
+                                      llvm::toString(file.takeError()));
+    } else
       error.Clear();
----------------
We should be consistent with the braces, both clauses are single-line. Personally I'd refactor the code and make this an early return instead.


================
Comment at: lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:187
       if (error.Fail() || !exe_module_sp) {
-        if (FileSystem::Instance().Readable(
-                resolved_module_spec.GetFileSpec())) {
+        if (FileSystem::Instance().Open(resolved_module_spec.GetFileSpec(),
+                                        lldb_private::File::eOpenOptionRead)) {
----------------
Not your fault, but it seems weird that we're doing this check after using the `resolved_module_spec` and only if it failed.. Shouldn't we try to read it first and stop early if that fails? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78712