jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed.
The substance seems fine. I'm not sure I would guess what GetInstructionsCount with canSetBreakpoint == true would do without reading the code. You could fix this with a more explicit parameter name, but I can't think of a good name that's short enough not to be awful, something like: `excludeInstructionsWhereYouCantSetBreakpoints` would get it but yuck... Better to just add a header comment explaining what that parameter actually does. It's also odd to have the SBInstructionList hold the logic determining whether you can set a breakpoint on an instruction. That should really be in SBInstruction, or even better should be in the lldb_private::Instruction class, since this seems like a generally useful bit of knowledge, and then routed through SBInstruction. https://reviews.llvm.org/D32168 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
