[Lldb-commits] [PATCH] D112632: [lldb] [Host/Terminal] Fix warnings with termios disabled

Fri Oct 29 00:29:27 PDT 2021

mgorny marked an inline comment as done.
mgorny added inline comments.

================
Comment at: lldb/source/Host/common/Terminal.cpp:39
+
 llvm::Expected<Terminal::Data> Terminal::GetData() {
   if (!FileDescriptorIsValid())
----------------
thakis wrote:
> nit: should GetData() even exist if `!LLDB_ENABLE_TERMIOS`? Looks like it's only called `#if LLDB_ENABLE_TERMIOS` now. Maybe the whole method definition should be inside an ifdef.
> 
> If it should keep existing, either move the !FileDescriptorIsValid() check into the #if inside it (imho preferred), or move all the callers of it outside that #if. The current mix is a bit self-inconsistent.
I think the goal was to avoid checking `LLDB_ENABLE_TERMIOS` in the header file, though I'm not sure if it was an explicit goal or one I've implicitly assumed.

I've move the check inside for the time being. We can always change it later.

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

https://reviews.llvm.org/D112632