JDevlieghere added inline comments.
================
Comment at: lldb/include/lldb/Target/Target.h:1451
+ /// Print all the signals set in this target.
+ void PrintDummySignals(Stream &strm, Args signals, size_t num_signals);
+
----------------
Was `Args` supposed to be a reference here? If not why do we need the copy?
I didn't look at the implementation yet, but why do we need `num_signals`?
Can't we count the number of args?
================
Comment at: lldb/include/lldb/Target/Target.h:1533
lldb::StackFrameRecognizerManagerUP m_frame_recognizer_manager_up;
+ std::map<std::string, DummySignalValues> m_dummy_signals;/// These are used
to
+ /// set the signal state when you don't have a process and more usefully
----------------
Does it matter that these are ordered? Can this use a `llvm::StringMap`?
================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1477-1479
+ bool only_target_values;
+ bool do_clear;
+ bool dummy;
----------------
Let's initialize these to the same values as `Clear`.
================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1582
bool DoExecute(Args &signal_args, CommandReturnObject &result) override {
- Target *target_sp = &GetSelectedTarget();
+ Target *target_sp = &GetSelectedOrDummyTarget();
----------------
Not your change but why `Target *` and not `Target &`?
================
Comment at: lldb/source/Target/Process.cpp:2931-2934
+ if (m_unix_signals_sp) {
+ StreamSP warning_strm = GetTarget().GetDebugger().GetAsyncErrorStream();
+ GetTarget().UpdateSignalsFromDummy(m_unix_signals_sp, warning_strm);
+ }
----------------
Clang format
================
Comment at: lldb/source/Target/Target.cpp:293-294
m_suppress_stop_hooks = false;
+ Args signal_args;
+ ClearDummySignals(signal_args);
}
----------------
Instead of having to pass an empty Args here, can we make the input argument an
`Optional<Args> = {}`? Then we can just call `ClearDummySignals` here. This
looks like `signal_args` is an output argument.
================
Comment at: lldb/source/Target/Target.cpp:3357
+void Target::AddDummySignal(const char *name, LazyBool pass, LazyBool notify,
+ LazyBool stop) {
----------------
It seems like this can take a `std::string`and `std::move` it into the pair
below. Or a `StringRef` if you turn this into a StringMap as per the other
suggestion.
================
Comment at: lldb/source/Target/Target.cpp:3366-3374
+ auto elem = m_dummy_signals.find(name);
+ if (elem != m_dummy_signals.end()) {
+ (*elem).second.pass = pass;
+ (*elem).second.notify = notify;
+ (*elem).second.stop = stop;
+ return;
+ }
----------------
With a StringMap you can simplify all of this into:
```
auto& entry = m_dummy_signals[name];
entry.pass = pass;
entry.notify = notify;
...
```
================
Comment at: lldb/source/Target/Target.cpp:3386-3389
+ if (elem.second.pass == eLazyBoolYes)
+ signals_sp->SetShouldSuppress(signo, false);
+ else if (elem.second.pass == eLazyBoolNo)
+ signals_sp->SetShouldSuppress(signo, true);
----------------
I'm wondering if we can simplify this with a little utility function. Something
like this:
```
static void helper(LazyBool b, std::function<void(bool)> f) {
switch(b) {
case eLazyBoolYes:
return f(true);
case eLazyBookFalse:
return f(false);
case eLazyBoolCalculate:
return;
}
llvm_unreachable("all cases handled");
}
```
That way we can simplify this:
```
helper(elem.second.pass, [](bool b) { signals_sp->SetShouldSuppress(signo, b);
});
```
================
Comment at: lldb/source/Target/Target.cpp:3436-3438
+ UnixSignalsSP signals_sp;
+ if (process_sp)
+ process_sp->GetUnixSignals();
----------------
Should this have been
```
if (process_sp)
signals_sp = process_sp->GetUnixSignals();
```
Now `signals_sp` is never initialized.
================
Comment at: lldb/source/Target/Target.cpp:3458-3464
+ auto str_for_lazy = [] (LazyBool lazy) -> const char * {
+ switch (lazy) {
+ case eLazyBoolCalculate: return "not set";
+ case eLazyBoolYes: return "true ";
+ case eLazyBoolNo: return "false ";
+ }
+ };
----------------
This seems super useful. Maybe this function, together with the other utility I
suggested, can go in a LazyBoolUtils or something in Utility.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126259/new/
https://reviews.llvm.org/D126259
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
