[Lldb-commits] [PATCH] D52651: Add functionality to export settings

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

To Raphael's points:

Argument completion handlers are set by the command object implementing HandleArgumentCompletion and dispatching to the CommandCompletions::eDiskFileCompletion.  An example of this is in CommandObjectProcessLaunch.  Note, arguments currently have a kind (as do options) but for historical reasons, those kinds don't automatically bind to completers.  Be nice to finish that bit of the design at some point...  But till then, this way though boilerplate is straightforward.

The StreamFile constructor doesn't handle ~.  Maybe it should?  Or you could make a FileSpec, they do handle ~ completion, and there's a StreamFile constructor that takes a FileSpec.

Breakpoints use "breakpoint write" and "breakpoint read" to save themselves.  Maybe it would be clearer if we use the same word for saving breakpoints and saving settings?

I also wonder if it would be good to add a "settings {import, read?}" command to go along with the settings {export,write}.  It is a little odd to do "settings export" to export the settings, and "command source" to read them back in.  That's relying on the accident that you are exporting the settings as commands, which (a) would be better not to tie ourselves to and (b) users shouldn't have to know that...

It also might be nice to have the output file be a -f option and then I could do:

(lldb) settings export -f foo.cmds

to export all settings and:

(lldb) settings export -f foo.cmds target

to export all the target commands, etc...  That might be more flexible for ordinary use.  If you do that, the easiest way to specify a completer is by using OptionGroupFile.



================
Comment at: source/Commands/CommandObjectSettings.cpp:213
+    }
+
     // Split the raw command into var_name and value pair.
----------------
"settings clear" also clears the settings value.  I'm not sure I'm in favor of having two ways of doing this, especially when the second one is undocumented.  Presumably this isn't intrinsic to exporting settings, so it should be done as a separate patch anyway.


================
Comment at: source/Commands/CommandObjectSettings.cpp:331
+    var_name_arg.arg_repetition = eArgRepeatOptional;
+
+    // There is only one variant this argument could be; put it into the
----------------
Shouldn't this be eArgRepeatPlain?  You don't do anything in the case where there's no filename argument.

BTW, I don't think these argument repeat counts are actually enforced.  They are used to write out the help strings, but not to check incoming arguments for commands so far as I can see.  The development of these and the argument kind specifications stalled when the person who originally worked on them left, and is waiting for somebody to pick them back up again.


================
Comment at: source/Commands/CommandObjectSettings.cpp:356
+    // Open file for dumping the exported settings.
+    const std::string export_path = args.GetArgumentAtIndex(0);
+    const uint32_t options = File::eOpenOptionWrite |
----------------
You don't know that you actually got an argument.  You should check that here.


https://reviews.llvm.org/D52651