[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 <reviews at 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.




> 
> 
> ================
> Comment at: source/Interpreter/OptionValueProperties.cpp:210-215
> +  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;
> ----------------
> 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 &is_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.