jingham accepted this revision.
jingham added a comment.
This looks good to me.
================
Comment at: lldb/source/API/SBDebugger.cpp:857
+ error = m_opaque_sp->GetTargetList().CreateTarget(
+ *m_opaque_sp, filename, arch, eLoadDependentsYes, platform_sp,
+ target_sp);
----------------
dblaikie wrote:
> jingham wrote:
> > clayborg wrote:
> > > I submit with "arc diff" and it will cause lint errors if I don't allow
> > > it to fix the lint errors it finds.
> > I'm 100% not in favor of tools that force irrelevant changes to be
> > included. But that is a suggested tool so somebody must like that.
> They aren't forced - you can submit with linter errors if they don't seem
> helpful.
>
> Pre-committing format changes in standalone NFC commits would generally be
> preferable. & the linter shouldn't be flagging untouched lines - is it?
It looks like the file had changes 700 lines before & 900 lines after, but
these lines weren't changed except by the linter...
================
Comment at: lldb/source/Core/Debugger.cpp:1188
+ (*pos)->ReportProgressPrivate(progress_id, message, completed, total,
+ is_debugger_specific);
+ }
----------------
jingham wrote:
> clayborg wrote:
> > In the Progress class I currently only store a pointer to the debugger if
> > one was specified. This means there is a lifetime issue that can arise. If
> > I do the callback for Debugger::ReportProgress(...) above, then we will
> > only notify debugger instances that are still around and in the global
> > list. If not then we need to either store a DebuggerSP or DebuggerWP in the
> > Progress object, which we can do if truly needed, but I don't like
> > DebuggerSP as I don't want a Progress to keep a debugger around. DebuggerWP
> > is possible if we really need this. Let me know what you think.
> > Debugger::ReportProgressPrivate() could be make into a static function if
> > needed so that no one sees it from Debugger.h.
> I think it's fine to drop progress notifications on the floor if they were
> debugger specific and that debugger is no longer around. I don't think it
> makes sense for the Progress to keep Debugger alive. As long as you check on
> the existence as you do here, you won't end up calling into a bad debugger,
> so the way you've done it looks fine. You might want to have an:
>
> if (is_debugger_specific) break;
>
> After the call to ReportProgressPrivate. It's unlikely we'll ever have
> enough debugger's that this a performance issue, but still looks weird to
> keep checking after we found the one we want.
>
> BTW, it might be better to have the Progress store the DebuggerID rather than
> the pointer. It would make it clear we aren't making claims about keeping
> the debugger alive.. That would simplify this code, since you could:
>
> if (debugger_id != LLDB_INVALID_DEBUGGER_ID) {
> DebuggerSP debugger_sp = Debugger::FindDebuggerWithID(debugger_id)
> if (debugger_sp) debugger_sp->ReportProgressPrivate(..., true);
> return;
> }
>
> Then your original loop just calling ReportProgressPrivate on all the
> debuggers, passing false for "debugger_specific".
BTW, I made up LLDB_INVALID_DEBUGGER_ID, apparently...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97739/new/
https://reviews.llvm.org/D97739
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
