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

[Phabricator via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL331315: Fix the .experimental. settings feature so that we 
dont return an error (authored by jmolenda, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D45348?vs=141473=144807#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D45348

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


Index: lldb/trunk/packages/Python/lldbsuite/test/settings/TestSettings.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/settings/TestSettings.py
+++ lldb/trunk/packages/Python/lldbsuite/test/settings/TestSettings.py
@@ -524,3 +524,41 @@
  "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)
Index: lldb/trunk/source/Interpreter/OptionValueProperties.cpp
===
--- lldb/trunk/source/Interpreter/OptionValueProperties.cpp
+++ lldb/trunk/source/Interpreter/OptionValueProperties.cpp
@@ -202,12 +202,23 @@
   llvm::StringRef value) {
   Status error;
   const bool will_modify = true;
+  llvm::SmallVector components;
+  name.split(components, '.');
+  bool name_contains_experimental = false;
+  for (const auto  : 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: lldb/trunk/packages/Python/lldbsuite/test/settings/TestSettings.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/settings/TestSettings.py
+++ 

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

[Pavel Labath via lldb-commits]

On Fri, 6 Apr 2018 at 23:36, Jason Molenda  wrote:

>
>
> > On Apr 6, 2018, at 2:07 AM, Pavel Labath via Phabricator <
> revi...@reviews.llvm.org> wrote:
> >
> > labath added inline comments.
> >
> >
> > 
> > Comment at:
> packages/Python/lldbsuite/test/settings/TestSettings.py:544-545
> > +# 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)
> > 
> > Isn't this basically what `self.expect` would do (only with better
> logging and error messages)?
>
>
> Ah, I didn't see that self.expect would allow me to specify whether to
> expect an error return or not.  Yes I can write this in terms of
> self.expect more cleanly.
>
> BTW what does the documentation for self.expect in lldbtest.py mean when
> it refers to "golden input"?  It uses the phrase a few times and I can't
> figure out what it's talking about.  Maybe that term was in the
> documentation from long ago and not a recent addition.
>
>
>
Thanks.

I think the self.expect function predates me, but if I had to guess, I
think here the term "golden" just means the "expected" output. The term
"golden output" is mostly used when comparing large files (usually images)
in tests. I suppose, with some imagination, it could be used checking the
verbatim output of a command is exactly a given string, but when we start
doing substring and regex matches it really becomes inappropriate... Maybe
originally the expect function only supported verbatim matches and this is
a leftover of that?
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 141473.
jasonmolenda added a comment.

rewrote the test cases in terms of self.expect.  Haven't looked at being 
totally correct with flagging paths with .experimental. as never-errored.


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 components;
+  name.split(components, '.');
+  bool name_contains_experimental = false;
+  for (const auto  : 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,41 @@
  "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)


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 components;
+  name.split(components, '.');
+  bool name_contains_experimental = false;
+ 

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

[Jason Molenda via lldb-commits]

> On Apr 6, 2018, at 2:07 AM, Pavel Labath via Phabricator 
>  wrote:
> 
> labath added inline comments.
> 
> 
> 
> Comment at: packages/Python/lldbsuite/test/settings/TestSettings.py:544-545
> +# 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)
> 
> Isn't this basically what `self.expect` would do (only with better logging 
> and error messages)?


Ah, I didn't see that self.expect would allow me to specify whether to expect 
an error return or not.  Yes I can write this in terms of self.expect more 
cleanly.

BTW what does the documentation for self.expect in lldbtest.py mean when it 
refers to "golden input"?  It uses the phrase a few times and I can't figure 
out what it's talking about.  Maybe that term was in the documentation from 
long ago and not a recent addition.




> 
> 
> 
> Comment at: source/Interpreter/OptionValueProperties.cpp:210-215
> +  llvm::SmallVector components;
> +  name.split(components, '.');
> +  bool name_contains_experimental = false;
> +  for (const auto  : components)
> +if (Properties::IsSettingExperimental(part))
> +  name_contains_experimental = true;
> 
> Not a big deal, but I would expect that the magicness of `experimental` would 
> kick in only for the components which are come after `experimental` keyword.
> So something like `target.experimental.non-existant-setting` should be 
> subject to the magic behavior, but 
> `target.non-existant-setting.experimental.foo` should not (?)


Yeah, I was debating whether to do it properly like that or not.  The 
GetSubValue() method is recursive and would need a new bool _experimental 
argument, but it would need to be initialized to false before GetSubValue was 
called, or GetSubValue would need to be renamed to GetSubValueImpl and a 
GetSubValue function that initializes it to false and then calls 
GetSubValueImpl would need to be done.  It's not terrible, but I couldn't make 
up my mind if it was worth the trouble to correctly error out on 
target.non-existant-setting.experimental.foo or not.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: packages/Python/lldbsuite/test/settings/TestSettings.py:544-545
+# 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)

Isn't this basically what `self.expect` would do (only with better logging and 
error messages)?



Comment at: source/Interpreter/OptionValueProperties.cpp:210-215
+  llvm::SmallVector components;
+  name.split(components, '.');
+  bool name_contains_experimental = false;
+  for (const auto  : components)
+if (Properties::IsSettingExperimental(part))
+  name_contains_experimental = true;

Not a big deal, but I would expect that the magicness of `experimental` would 
kick in only for the components which are come after `experimental` keyword.
So something like `target.experimental.non-existant-setting` should be subject 
to the magic behavior, but `target.non-existant-setting.experimental.foo` 
should not (?)


Repository:
  rL LLVM

https://reviews.llvm.org/D45348



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Jason Molenda via Phabricator via lldb-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 components;
+  name.split(components, '.');
+  bool name_contains_experimental = false;
+  for (const auto  : 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(), "")
+