[Lldb-commits] [PATCH] D47991: Improve SBThread's stepping API using SBError parameter.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

> I agree with Leonard, for the StepOver overload that returns errors, just make the mode parameter required. That will reduce confusion.

Works for me, too.



================
Comment at: source/API/SBThread.cpp:1136
   bool result = false;
   if (exe_ctx.HasThreadScope()) {
     Process::StopLocker stop_locker;
----------------
apolyakov wrote:
> @aprantl do you think that we should use early exit here? There is printing to log at the end of the function, so in case of early exit, we must duplicate `log->Printf()`. Is it worth it?
Duplicating the log message is probably not worth it.


================
Comment at: source/API/SBThread.cpp:1141
       result = true;
     } else {
       if (log)
----------------
It looks like the error string should be set here as well?


================
Comment at: source/API/SBThread.cpp:1146
     }
-  }
+  } else error.SetErrorString("this SBThread object is invalid");
   if (log)
----------------
I think clang-format would do this as:

```
} else
   error.SetErrorString("this SBThread object is invalid");
```


================
Comment at: source/API/SBThread.cpp:1171
     } else {
       if (log)
         log->Printf("SBThread(%p)::Resume() => error: process is running",
----------------
same here.


================
Comment at: source/API/SBThread.cpp:1179
                 static_cast<void *>(exe_ctx.GetThreadPtr()), result);
   return result;
 }
----------------
Looking at the implementation of SBThread::Resume and SBThread::Suspend, the only difference seems to be the API called and the log message. If there is a third command with a similar implementation, it might be a fun exercise to factor that out into a helper function that takes std::function<> and a log string argument...


https://reviews.llvm.org/D47991