[Lldb-commits] [PATCH] D45348: Don't return error for settings set .experimental. settings that are absent
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Fri Apr 6 15:36:02 PDT 2018
> 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.
More information about the lldb-commits