Page MenuHomePhabricator

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

Authored by mib on Jan 20 2021, 3:07 PM.

Details

Reviewers
JDevlieghere
jingham
teemperor
Group Reviewers
Restricted Project
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 <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.Jan 20 2021, 3:07 PM
mib requested review of this revision.Jan 20 2021, 3:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2021, 3:07 PM
mib edited the summary of this revision. (Show Details)Jan 20 2021, 3:08 PM

@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?

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 updated this revision to Diff 318072.Jan 20 2021, 5:27 PM

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

mib added a reviewer: Restricted Project.Jan 21 2021, 4:36 AM
mib edited the summary of this revision. (Show Details)Jan 21 2021, 5:37 AM
JDevlieghere accepted this revision as: JDevlieghere.Feb 9 2021, 9:52 AM

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

This revision is now accepted and ready to land.Feb 9 2021, 9:52 AM
mib closed this revision.Mar 3 2021, 10:02 PM

Landed in 103ad3f90