[Lldb-commits] [PATCH] D111355: [lldb] Add serial:// protocol for connecting to serial port

[Michał Górny via Phabricator via lldb-commits]

mgorny marked 3 inline comments as done.
mgorny added inline comments.


================
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp:772-774
+  llvm::Error error = llvm::Error::success();
+  m_io_sp = std::make_shared<SerialPort>(fd, File::eOpenOptionReadWrite,
+                                         serial_options.get(), true, error);
----------------
labath wrote:
> mgorny wrote:
> > labath wrote:
> > > mgorny wrote:
> > > > labath wrote:
> > > > > It would be better to hide this by making the constructor private and have a static factory function returning `Expected<unique_ptr<SerialPort>>`. Bonus points if you can move all the fallible operations into the factory function so that the (now private) constructor does not need the Error argument at all
> > > > I know but I couldn't make this work — the compiler just won't find a way to convert the new `SerialPort` instance into `Expected<SerialPort>`. I suspect I would have to add move ctors and assignment operators all the way up the class hierarchy.
> > > That's where the `unique_ptr` part comes in. :)
> > I presume you mean `shared_ptr` here, or know some dirty trick.
> a unique_ptr is convertible to a shared_ptr, but not the other way around, so it's better to return the former
Ah, indeed. I was missing std::move.


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

https://reviews.llvm.org/D111355