Re: [libvirt] [PATCH v2 2/3] util: enum: Add NULL 'label' arg VIR_ENUM_IMPL calls

On 4/16/19 5:11 AM, Michal Privoznik wrote: 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 IMO it's similar effort to identify places that need fixing: its grep of 'VIR_ENUM_IMPL_TYPE' vs 'VIR_ENUM_IMPL.*NULL'. 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