wallace added inline comments.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:77
bool TraceCursorIntelPT::IsError() {
return m_decoded_thread_sp->GetInstructions()[m_pos].IsError();
}
----------------
jj10306 wrote:
> zrthxn wrote:
> > jj10306 wrote:
> > > nit: should we update this to use the error map? I don't think there's a
> > > significant difference performance wise, but the code would be a little
> > > cleaner imo and consistent with how `GetError()` works.
> > That would sort of look like this I think
> >
> > ```
> > if
> > (m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos).isA<ErrorSuccess>())
> > return false;
> > else true;
> > ```
> What about
> `return (bool)m_decoded_thread_sp->GetErrorByInstructionIndex(m_pos);`
> Another idea is to just remove the `IsError()` function entirely since
> calling `GetError()` tells you if it's an error. iirc all error checks
> actually use `GetError` except for the checks inside of `TraceHtr` which is
> soon going to be deleted by @wallace in new patches, so you could just change
> those couple instances of `IsError` and remove it all together. Definitely
> not necessary, just spitballing ideas (:
> @wallace what do you think?
I don't think that's a good idea. The problem is calling
`GetErrorByInstructionIndex` is that you then have an Error object that you
need to consume. There's also the cost of creating this object even if you just
want to know if there's an error or not and you don't want to do anything with
the actual error message. It's better then to create the Error object only when
needed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122293/new/
https://reviews.llvm.org/D122293
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
