[Lldb-commits] [PATCH] D52772: [Settings] Make "settings set" without a value equivalent to "settings clear"

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added a reviewer: LLDB.

I want to make providing no value to `setting set <setting>` equivalent to clearing that setting: `settings clear <setting>`. The motivation is https://reviews.llvm.org/D52651 that allows settings to be written to and read from a file. Not all settings have a default or empty value that can be read again, for example enums where the default is not valid value (unknown for target.language) or strings where the empty string is not equal to an empty option.

- One possible alternative is giving every option an explicit and valid default. From a user perspective I don't think this is a good idea. For `target.language` it doesn't make sense to have `unknown` in the list of options.
- The alternative is changing all the dump methods to print `settings clear <setting>` for the `eDumpOptionCommand`. Personally I don't like adding too much logic to the dump methods.

I definitely share the feeling that it's unfortunate to have two methods of doing the same thing. However, I don't think this behavior is counter intuitive and a reasonable trade-off given the current situation.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52772

Files:
  source/Commands/CommandObjectSettings.cpp


Index: source/Commands/CommandObjectSettings.cpp
===================================================================
--- source/Commands/CommandObjectSettings.cpp
+++ source/Commands/CommandObjectSettings.cpp
@@ -38,7 +38,9 @@
 public:
   CommandObjectSettingsSet(CommandInterpreter &interpreter)
       : CommandObjectRaw(interpreter, "settings set",
-                         "Set the value of the specified debugger setting."),
+                         "Set the value of the specified debugger setting. "
+                         "Providing no value is equivalent to \"settings "
+                         "clear\"."),
         m_options() {
     CommandArgumentEntry arg1;
     CommandArgumentEntry arg2;
@@ -185,7 +187,7 @@
       return false;
 
     const size_t argc = cmd_args.GetArgumentCount();
-    if ((argc < 2) && (!m_options.m_global)) {
+    if ((argc < 1) && (!m_options.m_global)) {
       result.AppendError("'settings set' takes more arguments");
       result.SetStatus(eReturnStatusFailed);
       return false;
@@ -199,6 +201,18 @@
       return false;
     }
 
+    // A missing value corresponds to clearing the setting.
+    if (argc == 1) {
+      Status error(m_interpreter.GetDebugger().SetPropertyValue(
+          &m_exe_ctx, eVarSetOperationClear, var_name, llvm::StringRef()));
+      if (error.Fail()) {
+        result.AppendError(error.AsCString());
+        result.SetStatus(eReturnStatusFailed);
+        return false;
+      }
+      return result.Succeeded();
+    }
+
     // Split the raw command into var_name and value pair.
     llvm::StringRef raw_str(command);
     std::string var_value_string = raw_str.split(var_name).second.str();


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D52772.167891.patch
Type: text/x-patch
Size: 1685 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20181002/e029dd11/attachment.bin>