[Lldb-commits] [PATCH] D70907: Change Target::FindBreakpointsByName to use a vector

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/include/lldb/Breakpoint/BreakpointList.h:71
   ///   \bfalse if the input name was not a legal breakpoint name.
-  bool FindBreakpointsByName(const char *name, BreakpointList &matching_bps);
+  bool FindBreakpointsByName(const char *name, std::vector<lldb::BreakpointSP> &matching_bps);
 
----------------
JosephTremoulet wrote:
> JDevlieghere wrote:
> > I think the API would look nicer if we returned an `llvm::Optional<std::vector>>` where `None` means an invalid breakpoint name and an empty list no matches. What do you think?
> I think I'd go with `Expected<>` over `Optional<>`, since the `false` return indicates invalid input.
> 
> I actually originally considered different signatures for this change.  My first inclination was to switch the populated list from a `BreakpointList` to a `BreakpointIDList`, but it seemed that would be inefficient for the call from `Target::ApplyNameToBreakpoints` that needs the actual breakpoints.  So then I went down the route of `Expected<iterator_range<breakpoint iterator>>`, but it was quickly becoming more code (and more convoluted) than seemed warranted.  So I looked around, saw `std::vector`s being populated by e.g. `Breakpoint::GetNames` and `SourceManager::FindLinesMatchingRegex`, and decided to follow suit.
> 
> Which is a long way to say:  populating a `std::vector` seems to "fit in" with surrounding code better, but aside from that, yes I think returning `Expected<std::vector>` would be a more natural fit.  I don't know which of those concerns to prefer in this code; LMK and I'm happy to switch it if that seems best.
I think an `Expected` is perfect. I proposed `Optional` because the way things are, the call sites would have to discard the error anyway. However, given that `FindBreakpointsByName` takes a `Status`, I think it would be good to propagate that up. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70907