[Lldb-commits] [PATCH] D110721: [lldb] [Host] Refactor TerminalState

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Overall, this seems like an improvement, though the class is still quite far from what I would consider an ideal state.



================
Comment at: lldb/include/lldb/Host/Terminal.h:51
 /// descriptor and later restore that state as it originally was.
 class TerminalState {
 public:
----------------
I guess it would be good to delete the copy operations as well.


================
Comment at: lldb/include/lldb/Host/Terminal.h:20
+
 struct termios;
 
----------------
mgorny wrote:
> shafik wrote:
> > I don't think we need this forward declaration anymore.
> You are correct, thanks!
This is kind of a step backwards, as in llvm one tries to not expose system headers (https://llvm.org/docs/SupportLibrary.html#don-t-expose-system-headers). Obviously, lldb is quite far from that.

I think this is actually what the this code whas trying to achieve, though forward-declaring 3rd party entities is also a somewhat questionable practice. Pimpling it is the canonical solution, although it has its  (mostly mental) overhead.


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

https://reviews.llvm.org/D110721