Quiet command regex instructions during batch execution

[kastiglione]

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.

[jingham]

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]

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]

source/Commands/CommandObjectCommands.cpp

1012

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

[jingham]

Sure, that also sounds fine.

Jim

[kastiglione]

Added IOHandlerActivatedInteractively

[kastiglione]

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

[kastiglione]

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

[clayborg]

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());

[kastiglione]

Add interactive parameter

[kastiglione]

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

[kastiglione]

Thanks for reviewing @clayborg.

packages/Python/lldbsuite/test/functionalities/command_regex/TestCommandRegex.py

class CommandRegexTestCase(TestBase):
     @expectedFailureAll(
         hostoslist=["windows"],
         bugnumber="llvm.org/pr22274: need a pexpect replacement for windows")
     @no_debug_info_test
     def test_command_regex(self):
         """Test a simple scenario of 'command regex' invocation and subsequent use."""
         import pexpect
         prompt = "(lldb) "
-        regex_prompt = "Enter one of more sed substitution commands in the form: 's/<regex>/<subst>/'.\r\nTerminate the substitution list with an empty line.\r\n"
+        regex_prompt = "Enter one or more sed substitution commands in the form: 's/<regex>/<subst>/'.\r\nTerminate the substitution list with an empty line.\r\n"
         regex_prompt1 = "\r\n"
 
         child = pexpect.spawn('%s %s' %
                               (lldbtest_config.lldbExec, self.lldbOption))
         # Turn on logging for what the child sends back.
         if self.TraceOn():
             child.logfile_read = sys.stdout
         # So that the spawned lldb session gets shutdown durng teardown.

source/Commands/CommandObjectCommands.cpp

a number follows 'f':"
         R"(
 
     (lldb) command regex f s/^$/finish/ 's/([0-9]+)/frame select %1/')");
   }
 
   ~CommandObjectCommandsAddRegex() override = default;
 
 protected:
   void IOHandlerActivated(IOHandler &io_handler) override {
+    if (!io_handler.GetIsInteractive() && !io_handler.GetIsRealTerminal())
+      // Don't print instructions during batch execution, such as .lldbinit.
+      return;
     StreamFileSP output_sp(io_handler.GetOutputStreamFile());
     if (output_sp) {
-      output_sp->PutCString("Enter one of more sed substitution commands in "
+      output_sp->PutCString("Enter one or more sed substitution commands in "
                             "the form: 's/<regex>/<subst>/'.\nTerminate the "
                             "substitution list with an empty line.\n");
       output_sp->Flush();
     }
   }
 
   void IOHandlerInputComplete(IOHandler &io_handler,
                               std::string &data) override {