JosephTremoulet marked an inline comment as done. JosephTremoulet 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); ---------------- 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. 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
