[PATCH] D45348: Don't return error for settings set .experimental. settings that are absent

[Jason Molenda via Phabricator via llvm-commits]

jasonmolenda created this revision.
jasonmolenda added a reviewer: jingham.
jasonmolenda added a project: LLDB.
Herald added a subscriber: llvm-commits.

setting paths that include .experimental. are intended for settings that may be promoted to "real" settings in the future, or may be removed.  When users put these settings in their ~/.lldbinit files, we don't want to emit an error if the setting has gone away.  And if the setting has been promoted to a real setting, we want to do the right thing.

The first part of that -- not emitting an error -- did not work correctly.  I fixed that, and added tests to check all of these behaviors.


Repository:
  rL LLVM

https://reviews.llvm.org/D45348

Files:
  packages/Python/lldbsuite/test/settings/TestSettings.py
  source/Interpreter/OptionValueProperties.cpp


Index: source/Interpreter/OptionValueProperties.cpp
===================================================================
--- source/Interpreter/OptionValueProperties.cpp
+++ source/Interpreter/OptionValueProperties.cpp
@@ -207,12 +207,23 @@
                                           llvm::StringRef value) {
   Status error;
   const bool will_modify = true;
+  llvm::SmallVector<llvm::StringRef, 8> components;
+  name.split(components, '.');
+  bool name_contains_experimental = false;
+  for (const auto &part : components)
+    if (Properties::IsSettingExperimental(part))
+      name_contains_experimental = true;
+
+  
   lldb::OptionValueSP value_sp(GetSubValue(exe_ctx, name, will_modify, error));
   if (value_sp)
     error = value_sp->SetValueFromString(value, op);
   else {
-    if (error.AsCString() == nullptr)
+    // Don't set an error if the path contained .experimental. - those are
+    // allowed to be missing and should silently fail.
+    if (name_contains_experimental == false && error.AsCString() == nullptr) {
       error.SetErrorStringWithFormat("invalid value path '%s'", name.str().c_str());
+    }
   }
   return error;
 }
Index: packages/Python/lldbsuite/test/settings/TestSettings.py
===================================================================
--- packages/Python/lldbsuite/test/settings/TestSettings.py
+++ packages/Python/lldbsuite/test/settings/TestSettings.py
@@ -524,3 +524,51 @@
                              "target.process.extra-startup-command",
                              "target.process.thread.step-avoid-regexp",
                              "target.process.thread.trace-thread"])
+
+    # settings under an ".experimental" domain should have two properties:
+    #   1. If the name does not exist with "experimental" in the name path,
+    #      the name lookup should try to find it without "experimental".  So
+    #      a previously-experimental setting that has been promoted to a 
+    #      "real" setting will still be set by the original name.
+    #   2. Changing a setting with .experimental., name, where the setting
+    #      does not exist either with ".experimental." or without, should
+    #      not generate an error.  So if an experimental setting is removed,
+    #      people who may have that in their ~/.lldbinit files should not see
+    #      any errors.
+    def test_experimental_settings(self):
+        cmdinterp = self.dbg.GetCommandInterpreter()
+        result = lldb.SBCommandReturnObject()
+
+        # Set target.arg0 to a known value, check that we can retrieve it via
+        # the actual name and via .experimental.
+        cmdinterp.HandleCommand("settings set target.arg0 first-value", result)
+        self.assertEqual(result.Succeeded(), True)
+        cmdinterp.HandleCommand("settings show target.arg0", result)
+        self.assertEqual(result.Succeeded(), True)
+        self.assertTrue("first-value" in result.GetOutput())
+        cmdinterp.HandleCommand("settings show target.experimental.arg0", result)
+        self.assertEqual(result.Succeeded(), True)
+        self.assertTrue("first-value" in result.GetOutput())
+
+        # Set target.arg0 to a new value via a target.experimental.arg0 name,
+        # verify that we can read it back via both .experimental., and not.
+        cmdinterp.HandleCommand("settings set target.experimental.arg0 second-value", result)
+        self.assertEqual(result.Succeeded(), True)
+        cmdinterp.HandleCommand("settings show target.arg0", result)
+        self.assertEqual(result.Succeeded(), True)
+        self.assertTrue("second-value" in result.GetOutput())
+        cmdinterp.HandleCommand("settings show target.experimental.arg0", result)
+        self.assertEqual(result.Succeeded(), True)
+        self.assertTrue("second-value" in result.GetOutput())
+
+        # showing & setting an undefined .experimental. setting should generate no errors.
+        cmdinterp.HandleCommand("settings show target.experimental.setting-which-does-not-exist", result)
+        self.assertEqual(result.Succeeded(), True)
+        self.assertEqual(result.GetOutput().rstrip(), "")
+        cmdinterp.HandleCommand("settings set target.experimental.setting-which-does-not-exist true", result)
+        self.assertEqual(result.Succeeded(), True)
+
+        # finally, confirm that trying to seta setting that does not exist still fails.
+        # (SHOWING a setting that does not exist does not currently yield an error.)
+        cmdinterp.HandleCommand("settings set target.setting-which-does-not-exist true", result)
+        self.assertEqual(result.Succeeded(), False)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D45348.141263.patch
Type: text/x-patch
Size: 4627 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180406/32afc964/attachment.bin>