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)
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)
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits