jimingham wrote:
This is a good direction. The API for creating the error message seems fine, a
known part and a "site-specific error" is a workable division w/o being over
designed.
The one thing that bugs me about this whole setup is that we know the argument
type for each option, so even more of the stuff we're doing by hand here should
be automated. We should know the `--auto-continue` is a bool because we said
so when we defined the option, so it's awkward we have to intervene at all.
That's a bigger problem than you are addressing now.
The reason it comes up at all with this patch is that you also shouldn't need
to add the g_bool_parsing_error_message context by hand, we should know that
from the option type. I thought about that when I saw the code:
` if (!additional_context.empty())
stream << ": " << additional_context;
`
I was first thinking "we should put an assert there, it shouldn't be possible
to have an option parse fail and just say:
`Invalid value for -f (--foo)
`
That's really unhelpful." But at the same time I was thinking "wait, we know
the argument types, so in a lot of cases we could make a more helpful
conversion error." But that's currently fairly far from being plumbed through
to the point where you could use it.
In the end I think this is fine. As you work more on this it might be nice to
add some kind of trap to catch people who don't say why a value is wrong. And
far down the road, the Option error reporting could provide a useful fallback
for common option kinds like bool, int, etc.
But that can all be added. This is a fine way to get started.
https://github.com/llvm/llvm-project/pull/82273
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
