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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
