On 4/16/19 5:11 AM, Michal Privoznik wrote: > On 4/16/19 8:32 AM, Peter Krempa wrote: >> On Mon, Apr 15, 2019 at 17:26:41 -0400, Cole Robinson wrote: >>> Allow passing in a 'label' string for raising errors from >>> ToString/FromString calls. Adjust all VIR_ENUM_IMPL calls to >>> pass in NULL to disable error reporting. We will add strings at >>> a later time. >> >> I think that rather than changing every single VIR_ENUM_IMPL you should >> rather add VIR_ENUM_IMPL_TYPE or VIR_ENUM_IMPL_MSG or something like >> that which will allow to use the type string. >> This was the approach I took in the original RFC: https://www.redhat.com/archives/libvir-list/2018-July/msg01815.html I switched approaches for reasons I laid out in the last cover letter: https://www.redhat.com/archives/libvir-list/2019-April/msg00589.html Summary: I thought the original approach would make it more likely that we would end up in a half converted state. Then devs would need need to check whether use of a preexisting enum in new code is converted to raise an error or not, increasing the likely hood we accidentally double raise an error, or worse forget to raise an error when it's required. The v1 approach added error strings to all labels with the intent of turning on error reporting at a later time. Then the code would be fully converted, and we would just need to strip out the double error reporting which is the lesser of two evils than failing to raise an error IMO. Dan suggested the NULL approach and incremental conversion which is this v2. I still prefer the v1 approach (error strings all added, then turn on error reporting later). It will avoid the risk of new code forgetting to raise an error, worst case is double error reporting which is not that bad IMO. Once we turn on error reporting we can patch out ToString/FromString errors by whole file rather than individual enum, which will generate less patches that will be both easier to write and easier to review (no searching around the code to see if someone missed an enum usage). It front loads the interesting review part of determining the string labels rather than sprinkling them over potentially 200+ enum conversion patches. So I'm interested in both your thoughts on all the above >> That way it will also be far simpler to identify the places which need >> fixing. > IMO it's similar effort to identify places that need fixing: its grep of 'VIR_ENUM_IMPL_TYPE' vs 'VIR_ENUM_IMPL.*NULL'. > Yes, that sounds like a better idea. For instance: > > VIR_ENUM_IMPL_TYPE(virDomainVirt, > _("invalid domain type %s"), One of the benefits of centralizing error reporting is that we can reuse a single translated string with a common format. Requiring translation of every enum string is a waste IMO. And I don't think the label part should be translatable as it is largely referencing XML element names, or some API bit which is english only. I explained a bit more here: https://www.redhat.com/archives/libvir-list/2019-April/msg00814.html Thanks, Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
