[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Alexander Polyakov via Phabricator via lldb-commits]

apolyakov added inline comments.


================
Comment at: source/API/SBTarget.cpp:1467
+  }
+  const ConstString csFrom(from), csTo(to);
+  if (csFrom && csTo) {
----------------
aprantl wrote:
> apolyakov wrote:
> > aprantl wrote:
> > > personally I would write this as:
> > > ```
> > > if (!csFrom)
> > >   return error.SetErrorString("<from> path is empty");
> > > if (!csTo)
> > >   return error.SetErrorString("<to> path is empty");
> > > Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
> > > if (log)
> > >   log->Printf("SBTarget(%p)::%s: '%s' -> '%s'",
> > >               static_cast<void *>(target_sp.get()),  __FUNCTION__,
> > >               from, to);
> > > target_sp->GetImageSearchPathList().Append(csFrom, csTo, true);
> > > ```
> > in my opinion, the branch `if (csFrom && csTo)` will be executed in most cases, so it is reasonable to make it executed first, as this will benefit the branch prediction of the processor. Is this a worthwhile argument?
> Not really. First, you don't actually control the order in which the code is emitted by the compiler by rearranging the source code. Second, it doesn't make sense to think about branch prediction outside of a tight loop as the effect would not even be measurable. We should optimize for readability and compact code here.
Yes, I should agree that thinking about branch prediction isn't so effective in our case. But the readability of our approaches is almost equal. At the same time, taking into consideration that in most cases we will have not empty string arguments, your approach will check 2 conditions and mine approach will check 1 condition.


https://reviews.llvm.org/D49739