On Tue, Apr 29, 2025 at 15:27:44 +0200, Peter Krempa via Devel wrote:
> On Tue, Apr 29, 2025 at 12:19:49 +0200, Jiri Denemark via Devel wrote:
> > From: Jiri Denemark <jdene...@redhat.com>
> >
> > Refactor weight calculation to a separate virCPUx86WeightFeatures
> > function to avoid code duplication.
> >
> > Signed-off-by: Jiri Denemark <jdene...@redhat.com>
> > ---
> > 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.
Okay I see now that I've read 14/15 why this looks so confusing.
Please note in the commit message that the patch is faithfully
extracting the algorithm without modifying it even when it is wrong :D
>
> > + weight->disabled += penalty;
> > + penalty++;
> > + }
> > + }
> > +
> > + weight->total = weight->enabled + weight->disabled;
> > +}
> > +
> > +
>
> The new version is much more readable!
>
> Reviewed-by: Peter Krempa <pkre...@redhat.com>
>
