[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 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();
 }
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122293/new/

https://reviews.llvm.org/D122293