augusto2112 accepted this revision.
augusto2112 added a comment.
I have one nit that I'll leave up to you whether it should be addressed now or
not.
================
Comment at: lldb/source/Host/common/File.cpp:545
num_bytes = bytes_read;
- } else if (StreamIsValid()) {
+ } else if (ValueGuard file_lock = StreamIsValid()) {
bytes_read = ::fread(buf, 1, num_bytes, m_stream);
----------------
I believe `descriptor_guard` will still be in scope in the else branch (weird
scoping rules), but
================
Comment at: lldb/source/Host/common/File.cpp:609
num_bytes = bytes_written;
- } else if (StreamIsValid()) {
+ } else if (ValueGuard stream_guard = StreamIsValid()) {
bytes_written = ::fwrite(buf, 1, num_bytes, m_stream);
----------------
One thing that's not super obvious for people reading this is that
`descriptor_guard` is still in scope in the else branch (due to C++'s weird
scoping rules when it comes to declaring variables inside `if` statements), and
will keep the descriptor mutex locked which is probably not what you intended.
I don't think this affects the correctness of this implementation at the
moment, but could be dangerous with further changes on top of this one.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157347/new/
https://reviews.llvm.org/D157347
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
