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

[Alex Langford via lldb-commits]

bulbazord 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.

> 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