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

[walter erquinigo via Phabricator via lldb-commits]

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