andcarminati added inline comments.
================
Comment at: lldb/tools/driver/Driver.cpp:673-674
+static void sigtstp_handler(int signo) {
if (g_driver != nullptr)
g_driver->GetDebugger().SaveInputTerminalState();
----------------
labath wrote:
> JDevlieghere wrote:
> > andcarminati wrote:
> > > JDevlieghere wrote:
> > > > labath wrote:
> > > > > JDevlieghere wrote:
> > > > > > I see an opportunity for a little RAII helper.
> > > > > What kind of a helper did you have in mind? Practically the entire
> > > > > function consists of setup and teardown in preparation for the
> > > > > `raise(signo)` call. If I wanted to be fancy I could put all of that
> > > > > in a helper, but I don't think that would make it cleaner. Plus, we
> > > > > also need to be careful about the functions we call from a signal
> > > > > handler, and I really don't know whether e.g. `llvm::make_scope_exit`
> > > > > is guaranteed to not allocate (heap) memory.
> > > > I was only referring to the Save/RestoreInputTerminalState() part of
> > > > this function. Something like:
> > > >
> > > > ```
> > > > class TerminalStateRAII() {
> > > > public:
> > > > TerminalStateRAII(Driver* driver) : driver(m_driver) {
> > > > if (m_driver)
> > > > m_driver->GetDebugger().SaveInputTerminalState();
> > > > }
> > > >
> > > > ~SignalHelper() {
> > > > if (m_driver)
> > > > m_driver->GetDebugger().SaveInputTerminalState();
> > > > }
> > > >
> > > > private:
> > > > Driver* m_driver;
> > > > };
> > > > ```
> > > >
> > > > Obviously, this isn't at all important, just something that came to
> > > > mind.
> > > I think this is a good idea to reduce code duplication. Another approach:
> > >
> > > ```
> > > class TerminalStateRAII() {
> > > public:
> > > TerminalStateRAII(Driver* driver) : driver(m_driver) {
> > > SaveInputTerminalState();
> > > }
> > >
> > > ~TerminalStateRAII() {
> > > SaveInputTerminalState();
> > > }
> > >
> > > private:
> > > Driver* m_driver;
> > > void SaveInputTerminalState(){
> > > if (m_driver)
> > > m_driver->GetDebugger().SaveInputTerminalState();
> > > }
> > > };
> > >
> > > ```
> > That's a typo on my part, the destructor needs to call
> > `RestoreInputTerminalState` (as opposed to `SaveInputTerminalState`).
> Ok, I see. If this was a more complex function (e.g. multiple return points),
> then I'd agree, but this function is really simple and linear (just like a
> signal handler should be). I am not convinced by the "code duplication"
> argument -- the way I see it, the helper class replaces 4 lines of (simple)
> code with ~15 lines of boilerplate. And this function is literally the only
> caller of Save/RestoreInputTerminalState.
Considering this argument:
> Ok, I see. If this was a more complex function (e.g. multiple return points),
> then I'd agree, but this function is really simple and linear (just like a
> signal handler should be).
LGTM
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120320/new/
https://reviews.llvm.org/D120320
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
