[lldb/Commands] Fix short option collision for `process launch`

[mib]

Summary

This patch changes the short option used in CommandOptionsProcessLaunch for the -v|--environment command option to -E|--environment.

The reason for that is, that it collides with the -v|--structured-data-value
command option generated by OptionGroupPythonClassWithDict that
I'm using in an upcoming patch for the process launch command.

The long option --environment remains the same.

Signed-off-by: Med Ismail Bennani <[e-mail]>

[JDevlieghere]

@jingham should weigh in here, but I think we kind of guarantee (at least informally) that the command options to be stable. Is there an alternative way to fix this without breaking the existing option?

[jingham]

We shouldn't lightly change options that are commonly used. But we haven't made a strong statement about this the way we do for API. I'm more concerned about people having to rewrite/build and distribute their code to accommodate a new lldb than having to learn a new character for a not very commonly used option.

I think it's more common to run env FOO=Bar or set the env-vars. And building up complex "process launch" commands doesn't seem like the best way to use lldb in a scripted manner. So this seems like a pretty benign change to me.

I would really like it if wherever we are introducing a scripted plugin to lldb, we pass its arguments in the same way. Since we're passing in dictionaries as the most flexible way to muster configuration info into the script interpreter, so --key --value are the most natural options, and so -k and -v... I'd really rather not have this be different across commands. OTOH -v doesn't really scream "environment" and we don't use that as an option anywhere else, so it seems more natural to change --environment's short option. Might be good to change it to -E not -V. -e wasn't free because we use it for stderr everywhere, but -E is free and makes more sense.

But it might be worthwhile to bring this up on lldb-dev to see if anybody has reasons not to do this that I haven't thought about?

[mib]

Changed short option to -E|--environment as @jingham suggested it.

[JDevlieghere]

LGTM, since nobody raised any concerns I think this is fine to land.

[mib]

Landed in 103ad3f90

lldb/docs/use/map.rst

<table class="mapping" cellspacing="0">
 
          <tr>
                <td class="header" colspan="2">Set environment variables for process and launch process in one command.</td>
          </tr>
          <tr>
                <td class="content">
                </td>
                <td class="content">
-                  <b>(lldb)</b> process launch -v DEBUG=1
+                  <b>(lldb)</b> process launch -E DEBUG=1
                   <br>
                </td>
          </tr>
 
          <tr>
                <td class="header" colspan="2">Attach to a process with process ID 123.</td>
          </tr>
          <tr>

lldb/source/Commands/CommandOptionsProcessLaunch.cpp

Status CommandOptionsProcessLaunch::SetOptionValue(
 
   case 'c':
     if (!option_arg.empty())
       launch_info.SetShell(FileSpec(option_arg));
     else
       launch_info.SetShell(HostInfo::GetDefaultShell());
     break;
 
-  case 'v':
+  case 'E':
     launch_info.GetEnvironment().insert(option_arg);
     break;
 
   default:
     error.SetErrorStringWithFormat("unrecognized short option character '%c'",
                                    short_option);
     break;
   }
   return error;
 }
 
 llvm::ArrayRef<OptionDefinition> CommandOptionsProcessLaunch::GetDefinitions() {
   return llvm::makeArrayRef(g_process_launch_options);
 }

lldb/source/Commands/Options.td

let Command = "process launch" in {
   def process_launch_disable_aslr : Option<"disable-aslr", "A">, Arg<"Boolean">,
     Desc<"Set whether to disable address space layout randomization when launching a process.">;
   def process_launch_plugin : Option<"plugin", "P">, Arg<"Plugin">,
     Desc<"Name of the process plugin you want to use.">;
   def process_launch_working_dir : Option<"working-dir", "w">, Arg<"DirectoryName">,
     Desc<"Set the current working directory to <path> when running the inferior.">;
   def process_launch_arch : Option<"arch", "a">, Arg<"Architecture">,
     Desc<"Set the architecture for the process to launch when ambiguous.">;
-  def process_launch_environment : Option<"environment", "v">,
+  def process_launch_environment : Option<"environment", "E">,
     Arg<"None">, Desc<"Specify an environment variable name/value string "
     "(--environment NAME=VALUE). Can be specified multiple times for subsequent "
     "environment entries.">;
   def process_launch_shell : Option<"shell", "c">, GroupRange<1,3>,
     OptionalArg<"Filename">, Desc<"Run the process in a shell (not supported on all platforms).">;
   def process_launch_stdin : Option<"stdin", "i">, Group<1>,
     Arg<"Filename">, Desc<"Redirect stdin for the process to <filename>.">;
   def process_launch_stdout : Option<"stdout", "o">, Group<1>,