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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
