[Lldb-commits] [lldb] [lldb][NFCI] Minor refactor to CommandObjectProcessHandle::VerifyCommandOptionValue (PR #79901)

[via lldb-commits]

jimingham wrote:

> > This version is still a little awkward because there's an ambiguity about what the incoming string being empty means. In the VerifyCommandOptionValue, it means "No Value" and the return is not distinguishable from "error" - but in a bunch of the places where you use VerifyCommandOptionValue an empty string just means a value wasn't provided, so you end up with constructs where you set the optional (checking the incoming string for `empty`) then check the input string again so you can tell empty apart from error, THEN make up the error on the outside w/o knowing what actually was wrong with the string.
> 
> I did this intentionally so that `VerifyCommandOptionValue` would not attempt to interpret the result. The caller knows whether or not its input is empty, so it's able to interpret the results of `VerifyCommandOptionValue` however it wants. Maybe `VerifyCommandOptionValue` is not a good name for this? It seems to just be trying to take a string and trying to get a bool out of it. Thinking about it further, I'm starting to question the value of `VerifyCommandOptionValue`... Maybe we should be using `OptionArgParser::ToBoolean`? That seems to be the end goal anyway.

By treating "empty string" exactly the same as "parse error" this kind of is interpreting the results, requiring a "pre-interpretation" to shadow this.

But the only thing VerifyCommandOptionValue adds in this case is treating 0 => false and 1 => true, and it's not at all clear to me why that isn't the job of ToBoolean.  And that API is weird anyway, it takes an input string, checks whether it can reasonably be interpreted as a boolean (including 0->false, 1->true) then converts the boolean value back to an integer value of 0 or 1.  It's very possible this is my doing, I can't remember, but that is kind of odd to begin with.  And why is it only used for these three booleans and no others?

I think getting rid of it and just using ToBoolean seems the better path.

> 
> > I wonder if it would be better to add a Status to the call. Then empty input -> "Empty Optional but no error"; error in parsing -> "Empty Optional but error" and result means success. Then you wouldn't have to do two checks on the input string in different places, or make up the error string.



https://github.com/llvm/llvm-project/pull/79901