kastiglione added inline comments.

================
Comment at: lldb/source/Commands/Options.td:232
+    Desc<"Delete all breakpoints which are currently disabled.  When using the 
disabled option "
+    "any breakpoints listed on the command line are EXCLUDED from deletion.">;
 }
----------------
jingham wrote:
> kastiglione wrote:
> > jingham wrote:
> > > kastiglione wrote:
> > > > jingham wrote:
> > > > > kastiglione wrote:
> > > > > > jingham wrote:
> > > > > > > kastiglione wrote:
> > > > > > > > To me, it's counter intuitive that `break delete --disabled 1` 
> > > > > > > > will not delete bp 1.
> > > > > > > The combination:
> > > > > > > 
> > > > > > > (lldb) break delete --disabled 1
> > > > > > > 
> > > > > > > could either mean 
> > > > > > > 
> > > > > > > 1) delete all breakpoints that are disabled AND breakpoint 1
> > > > > > > 2) delete all breakpoints that are disabled EXCEPT breakpoint 1
> > > > > > > 3) an error
> > > > > > > 
> > > > > > > Of those interpretations, 1 and 3 don't seem very useful, but 2 
> > > > > > > does.  
> > > > > > > 
> > > > > > > This is particularly handy when you specify a breakpoint name, 
> > > > > > > not a breakpoint.  Just make breakpoints you don't want deleted 
> > > > > > > DoNotDelete, then you can easily protect all those breakpoints.  
> > > > > > > 
> > > > > > > Note, your workaround would only be useful in this case if all 
> > > > > > > the breakpoints named DoNotDelete are currently disabled.  
> > > > > > > Otherwise you would have to remember which of the DoNotDelete 
> > > > > > > breakpoints were disabled, enable them all, do the `delete 
> > > > > > > --disabled` then  only re-disable those that were originally 
> > > > > > > disabled.  Whereas if you can pass an exclude list you can just 
> > > > > > > protect those breakpoints unconditionally regardless of their 
> > > > > > > state.
> > > > > > > 
> > > > > > > So while I agree this is a little odd, it's actually the only 
> > > > > > > option that really makes sense, it's easy to document, and I 
> > > > > > > don't think it's likely to cause mistakes. 
> > > > > > why does the first interpretation not seem useful? If I'm deleting 
> > > > > > breakpoints, I might want to delete both disabled breakpoints and 
> > > > > > one or more specific breakpoints. To do that I would probably 
> > > > > > intuitively write `break delete --disabled OthersToDelete`.
> > > > > > 
> > > > > > Could the ambiguity be removed by adding another flag? `break 
> > > > > > delete --disabled --except DoNotDelete`?
> > > > > To me "delete --disabled" is a bulk operation acting on a class of 
> > > > > breakpoints.  "This class plus one random other one" seems odd to me. 
> > > > >  
> > > > > 
> > > > > A bulk operation with exclusions makes much more sense to me.  
> > > > > 
> > > > > Adding another option complicates things without adding much value, 
> > > > > and becomes annoying if you want to specify more than one excluded 
> > > > > thing.  It would be easy to make the mistake:
> > > > > 
> > > > > (lldb) break disable --disabled --except 1 2
> > > > > 
> > > > > intending to preserve 2 but in fact deleting it.
> > > > I get that exclusions are useful, my concern is that the command 
> > > > "breakpoint delete" doesn't delete what you give it. If `break delete 
> > > > foo` deletes foo, then on the surface `break delete --disabled foo` 
> > > > should also delete foo. The flag does what it says, but also silently 
> > > > inverts the meaning of the positional args.
> > > The help for the option explicitly says that it inverts the meaning of 
> > > the positional args, there's nothing silent about it.  You wouldn't 
> > > accidentally say `break delete --disabled`, so presumably you would have 
> > > to have read the help for the option, which I don't think is susceptible 
> > > to misconstruction.
> > > 
> > > Because of that, I'm not too bothered that `break delete --disabled Foo` 
> > > behaves differently from `break delete Foo`.  And it seems the simplest 
> > > way to express the most useful thing you would want to add to just`break 
> > > delete --disable`.
> > In my experience people learn about lldb through 
> > twitter/coworkers/blogs/talks/tutorials etc, and not through `help`. Of 
> > those who learn from help, they may not read every word. It's quite 
> > possible to use this flag without having read the fine print.
> Given that misusing this command+option would result in a breakpoint NOT 
> getting deleted, I'm less concerned about the possibility of misuse.  The 
> reaction is "I did a somewhat odd thing and the odd bit I added didn't work 
> as expected (in a non-destructive way) so maybe I should read the help".  
> That doesn't seem problematic to me.
👍 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88129

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to