[Settings] Make "settings set" without a value equivalent to "settings clear"

[JDevlieghere]

Summary

I want to make providing no value to setting set <setting> equivalent to clearing that setting: settings clear <setting>. The motivation is 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.

[jingham]

Would it be possible for the exporter to notice empty settings and write "settings clear" instead? I'm worried that if you have a complicated setting, and the you do:

(lldb) settings set target.some-complex-setting

and decide you are wrong, you don't want to change the complex setting, then you have to know to delete the text - hitting a return is actually destructive.

[JDevlieghere]

In D52772#1252656, @jingham wrote:

Would it be possible for the exporter to notice empty settings and write "settings clear" instead?

I don't think we can if we want to re-use the existing dump infrastructure, unless there's a way to ask a Property if is empty/has a default value?

I'm worried that if you have a complicated setting, and the you do:

(lldb) settings set target.some-complex-setting

and decide you are wrong, you don't want to change the complex setting, then you have to know to delete the text - hitting a return is actually destructive.

This sounds like a very specific issue, though. You have the same problem if the value is something like a string and the user accidentally types one more character. What kind of scenario do you have in mind where a user would be able to type the command but not know how to delete it? I'm definitely not saying it's not a valid reason to consider alternatives and the current behavior is nice, but do you consider this a showstopper?

[jingham]

I'm thinking of the scenario where you type:

(lldb) settings set target.run-args

and then decide you didn't want to change the run-args after all. Before this change hitting return used to be a way to dismiss the settings operation, and that actually seems a pretty natural thing to do - especially given that there is a "clear" operation.

[jingham]

And more, I just like operations to be explicit and not have overloads like "settings set property" == "settings clear property".

[JDevlieghere]

Only clear setting when the force flag is set (as suggested offline by @jingham).

[JDevlieghere]

ping

[jingham]

That looks fine.

lit/Settings/TestSettingsSet.test

+# This tests setting setting values.
+
+# Check that setting an empty value with -f(orce) clears the value.
+# RUN: %lldb -b -s %s 2>&1 | FileCheck %s
+
+settings set tab-size 16
+settings show tab-size
+# CHECK: tab-size (unsigned) = 16
+
+settings set -f tab-size
+settings show tab-size
+# CHECK: tab-size (unsigned) = 4
+
+settings set tab-size
+# CHECK: error: 'settings set' takes more arguments

source/Commands/CommandObjectSettings.cpp

 using namespace lldb_private;
 
 //-------------------------------------------------------------------------
 // CommandObjectSettingsSet
 //-------------------------------------------------------------------------
 
 static constexpr OptionDefinition g_settings_set_options[] = {
     // clang-format off
-  { LLDB_OPT_SET_2, false, "global", 'g', OptionParser::eNoArgument, nullptr, {}, 0, eArgTypeNone, "Apply the new value to the global default value." }
+  { LLDB_OPT_SET_2, false, "global", 'g', OptionParser::eNoArgument, nullptr, {}, 0, eArgTypeNone, "Apply the new value to the global default value." },
+  { LLDB_OPT_SET_2, false, "force",  'f', OptionParser::eNoArgument, nullptr, {}, 0, eArgTypeNone, "Force an empty value to be accepted as the default." }
     // clang-format on
 };
 
 class CommandObjectSettingsSet : public CommandObjectRaw {
 public:
   CommandObjectSettingsSet(CommandInterpreter &interpreter)
       : CommandObjectRaw(interpreter, "settings set",
                          "Set the value of the specified debugger setting."),
public:
     ~CommandOptions() override = default;
 
     Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
                           ExecutionContext *execution_context) override {
       Status error;
       const int short_option = m_getopt_table[option_idx].val;
 
       switch (short_option) {
+      case 'f':
+        m_force = true;
+        break;
       case 'g':
         m_global = true;
         break;
       default:
         error.SetErrorStringWithFormat("unrecognized options '%c'",
                                        short_option);
         break;
       }
 
       return error;
     }
 
     void OptionParsingStarting(ExecutionContext *execution_context) override {
       m_global = false;
+      m_force = false;
     }
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
       return llvm::makeArrayRef(g_settings_set_options);
     }
 
     // Instance variables to hold the values for command options.
-
     bool m_global;
+    bool m_force;
   };
 
   int HandleArgumentCompletion(
       CompletionRequest &request,
       OptionElementVector &opt_element_vector) override {
 
     const size_t argc = request.GetParsedLine().GetArgumentCount();
     const char *arg = nullptr;
protected:
   bool DoExecute(llvm::StringRef command,
                  CommandReturnObject &result) override {
     Args cmd_args(command);
 
     // Process possible options.
     if (!ParseOptions(cmd_args, result))
       return false;
 
+    const size_t min_argc = m_options.m_force ? 1 : 2;
     const size_t argc = cmd_args.GetArgumentCount();
-    if ((argc < 2) && (!m_options.m_global)) {
+
+    if ((argc < min_argc) && (!m_options.m_global)) {
       result.AppendError("'settings set' takes more arguments");
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
 
     const char *var_name = cmd_args.GetArgumentAtIndex(0);
     if ((var_name == nullptr) || (var_name[0] == '\0')) {
       result.AppendError(
           "'settings set' command requires a valid variable name");
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
 
+    // A missing value corresponds to clearing the setting when "force" is
+    // specified.
+    if (argc == 1 && m_options.m_force) {
+      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();
     const char *var_value_cstr =
         Args::StripSpaces(var_value_string, true, false, false);
 
     Status error;
     if (m_options.m_global) {