This is an archive of the discontinued LLVM Phabricator instance.

Quiet command regex instructions during batch execution
ClosedPublic

Authored by kastiglione on Jun 28 2018, 4:38 PM.

Details

Summary

Within .lldbinit, regex commands can be structured as a list of substitutions over
multiple lines. It's possible that this is uninentional, but it works and has
benefits.

For example:

command regex <command-name>
s/pat1/repl1/
s/pat2/repl2/
...

I use this form of command regex in my ~/.lldbinit, because it makes it
clearer to write and read compared to a single line definition, because
multiline substitutions don't need to be quoted, and are broken up one per line.

However, multiline definitions result in usage instructions being printed for
each use. The result is that every time I run lldb, I get a dozen or more
lines of noise. With this change, the instructions are only printed when
command regex is invoked interactively, or from a terminal, neither of which
are true when lldb is sourcing ~/.lldbinit.

Event Timeline

kastiglione created this revision.Jun 28 2018, 4:38 PM

This behavior change seems desirable. But there are other commands that use the IOHandlerActivated method to print instructions like this, and should get the same treatment (breakpoint command add, type summary add...). It seems ugly to have to do this one by one in individual commands.

The IOHandlerDelegate is a pretty general class, so I don't think it is right for the IOHandler to only dispatch "IOHandlerActivated" to it when interactive. There's no requirement that the work you do in this method is only useful when interactive. But it might work if the IOHandlerDelegate could say whether it wants to be notified for IOHandlerActivated when non-interactive (like have a "ShouldNotifyWhenNonInteractive" method), and then the IOHandler could dispatch it or not based on the response to that method.

kastiglione added a comment.EditedJun 29 2018, 11:55 AM

Thanks @jingham.

if the IOHandlerDelegate could say whether it wants to be notified for IOHandlerActivated when non-interactive (like have a "ShouldNotifyWhenNonInteractive" method), and then the IOHandler could dispatch it or not based on the response to that method

So the "ShouldNotifyWhenNonInteractive" would only control whether IOHandlerActivated is called, not any of the other IOHandlerDelegate methods?

What do you think about adding a IOHandlerActivatedInteractivelydelegate method? This could be called in addition to IOHandlerActivated, but only for interactive IO?

clayborg added inline comments.Jul 2 2018, 7:35 AM
source/Commands/CommandObjectCommands.cpp
1012

Why do we need to check for a real terminal here? Isn't checking if it is interactive enough?

Sure, that also sounds fine.

Jim

Added IOHandlerActivatedInteractively

There are other commands that print instructions in the same way that command regex does, should they be updated in this change too?

@jingham If you're still willing to review this change to command regex, it is updated per our previous discussion, thanks.

clayborg requested changes to this revision.Nov 28 2018, 11:43 AM

Seems like we should just add a "bool interactive" as a second parameter to "IOHandlerActivated". Then it will be easy to find the other places that need to be fixed up.

include/lldb/Core/IOHandler.h
201 ↗(On Diff #172488)

Maybe remove the function below and add a "bool interactive" as a second parameter to this function?

source/Commands/CommandObjectCommands.cpp
1011–1014

See above inline comment about leaving the name the same but adding the "bool interactive" as a paramter

1016

If we make changes I requested above this would become:

if (output_sp && interactive)
source/Core/IOHandler.cpp
335–337 ↗(On Diff #172488)
m_delegate. IOHandlerActivated(*this, GetIsInteractive());
This revision now requires changes to proceed.Nov 28 2018, 11:43 AM

Add interactive parameter

kastiglione marked 4 inline comments as done.Feb 16 2019, 12:20 AM
kastiglione marked an inline comment as done.

@clayborg this has been updated the approach you suggested, adding an interactive parameter.

clayborg accepted this revision.Feb 27 2019, 11:32 AM
This revision is now accepted and ready to land.Feb 27 2019, 11:32 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2019, 4:17 PM
Herald added a subscriber: abidh. · View Herald Transcript

Thanks for reviewing @clayborg.