From b81afd6598fbd50e0bc09c6825db5f3fd3b3c9da Mon Sep 17 00:00:00 2001 From: Jason Molenda Date: Tue, 1 May 2018 22:49:01 +0000 Subject: [PATCH] Fix the .experimental. settings feature so that we don't return an error if an experimental setting has been removed/is missing. Add tests for the .experimental. settings behaviors -- that they correctly forward through to the real setting if it has become a real setting, that they don't generate errors when a settig has been removed. As Pavel notes in https://reviews.llvm.org/D45348, the way I'm suppressing errors in the setting is not completely correct - if any of the setting path components include "experimental", a missing setting would be declared a non-error. So settings set target.experimental.setting-that-does-not-exist true would not generate an error, which is correct. But as Pavel notes, settings set setting-does-not-exist.experimental.run-stopped true should generate an error because the unknown name occurs before the "experimental". The amount of change to do this correctly hasn't thrilled me, so I'm leaving this as-is for now. Differential Revision: https://reviews.llvm.org/D45348 llvm-svn: 331315 --- .../lldbsuite/test/settings/TestSettings.py | 38 +++++++++++++++++++ .../Interpreter/OptionValueProperties.cpp | 13 ++++++- 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/lldb/packages/Python/lldbsuite/test/settings/TestSettings.py b/lldb/packages/Python/lldbsuite/test/settings/TestSettings.py index 715cbab7c2a3..6b8ac7c3d5d5 100644 --- a/lldb/packages/Python/lldbsuite/test/settings/TestSettings.py +++ b/lldb/packages/Python/lldbsuite/test/settings/TestSettings.py @@ -524,3 +524,41 @@ class SettingsCommandTestCase(TestBase): "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. + self.expect('settings set target.arg0 first-value') + self.expect('settings show target.arg0', substrs=['first-value']) + self.expect('settings show target.experimental.arg0', substrs=['first-value'], error=False) + + # 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. + self.expect('settings set target.experimental.arg0 second-value', error=False) + self.expect('settings show target.arg0', substrs=['second-value']) + self.expect('settings show target.experimental.arg0', substrs=['second-value'], error=False) + + # showing & setting an undefined .experimental. setting should generate no errors. + self.expect('settings show target.experimental.setting-which-does-not-exist', patterns=['^\s$'], error=False) + self.expect('settings set target.experimental.setting-which-does-not-exist true', error=False) + + # A domain component before .experimental. which does not exist should give an error + # But the code does not yet do that. + # self.expect('settings set target.setting-which-does-not-exist.experimental.arg0 true', error=True) + + # finally, confirm that trying to set a setting that does not exist still fails. + # (SHOWING a setting that does not exist does not currently yield an error.) + self.expect('settings set target.setting-which-does-not-exist true', error=True) diff --git a/lldb/source/Interpreter/OptionValueProperties.cpp b/lldb/source/Interpreter/OptionValueProperties.cpp index 3a11028192c2..c1887f34b712 100644 --- a/lldb/source/Interpreter/OptionValueProperties.cpp +++ b/lldb/source/Interpreter/OptionValueProperties.cpp @@ -202,12 +202,23 @@ Status OptionValueProperties::SetSubValue(const ExecutionContext *exe_ctx, llvm::StringRef value) { Status error; const bool will_modify = true; + llvm::SmallVector 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; }