Re: [PATCH 14/15] cpu_x86: Refactor virCPUx86CompareCandidateFeatureList

On Tue, Apr 29, 2025 at 12:19:49 +0200, Jiri Denemark via Devel wrote: > From: Jiri Denemark <jdenemar(a)redhat.com&gt; > > Refactor weight calculation to a separate virCPUx86WeightFeatures > function to avoid code duplication. > > Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; > --- > src/cpu/cpu_x86.c | 83 ++++++++++++++++++++++++++--------------------- > 1 file changed, 46 insertions(+), 37 deletions(-) > > diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c > index 32aa01bc14..b0fe2bed4c 100644 > --- a/src/cpu/cpu_x86.c > +++ b/src/cpu/cpu_x86.c > @@ -2090,63 +2090,72 @@ virCPUx86Compare(virCPUDef *host, > /* Base penalty for disabled features. */ > #define BASE_PENALTY 2 > > +struct virCPUx86Weight { > + size_t total; > + size_t enabled; > + size_t disabled; > +}; > + > +static void > +virCPUx86WeightFeatures(const virCPUDef *cpu, > + struct virCPUx86Weight *weight) > +{ > + int penalty = BASE_PENALTY; > + size_t i; > + > + weight->enabled = cpu->nfeatures; > + weight->disabled = 0; > + > + if (cpu->type == VIR_CPU_TYPE_HOST) { > + weight->total = cpu->nfeatures; > + return; > + } > + > + for (i = 0; i < weight->enabled; i++) { > + if (cpu->features[i].policy == VIR_CPU_FEATURE_DISABLE) { > + weight->enabled--; I know that this is just moving code but the fact that you modify the variable used to limit the iteration ought to be explained in a comment because it's really non-obvious to a reader what's happening here.

> + weight->disabled += penalty; > + penalty++; > + } > + } > + > + weight->total = weight->enabled + weight->disabled; > +} > + > + The new version is much more readable! Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;