qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH for-1.4 qom-cpu 6/9] pc: Set fw_cfg data based o


From: Eduardo Habkost
Subject: Re: [Qemu-devel] [PATCH for-1.4 qom-cpu 6/9] pc: Set fw_cfg data based on APIC ID calculation
Date: Wed, 23 Jan 2013 15:46:04 -0200
User-agent: Mutt/1.5.21 (2010-09-15)

On Wed, Jan 23, 2013 at 06:32:29PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <address@hidden> writes:
> 
> > On Wed, Jan 23, 2013 at 05:33:25PM +0100, Andreas Färber wrote:
> >> Am 22.01.2013 21:25, schrieb Eduardo Habkost:
> [...]
> >> > +    for (cpu_idx = 0; cpu_idx < max_cpus; cpu_idx++) {
> >> > +        unsigned int apic_id = x86_cpu_apic_id_from_index(cpu_idx);
> >> > +        assert(apic_id < apic_id_limit);
> >> >          for (j = 0; j < nb_numa_nodes; j++) {
> >> > -            if (test_bit(i, node_cpumask[j])) {
> >> > -                numa_fw_cfg[i + 1] = cpu_to_le64(j);
> >> > +            if (test_bit(cpu_idx, node_cpumask[j])) {
> >> > +                numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);
> >> >                  break;
> >> >              }
> >> >          }
> >> >      }
> >> 
> >> Why can't we keep using i here? That would leave the "for (..." and
> >> "test_bit" lines unchanged and let us spot the actual changes of i vs.
> >> apic_id more easily.
> >
> > It would make the patch simpler, but at the cost of keeping variable
> > names opaque for people reading the code in the future. I believe
> > readable code is more important than making patches smaller.
> 
> Both are important, and you can have both at the same time: put the
> rename in its own patch.  Whether that's justified in this case is
> debatable.

I like making small patches and do everything in very small steps. But I
learned that in practice this was making my patches take forever to be
reviewed, and I often got questions like "why don't you squash this with
the next/previous patch?" and "why are you touching this code that is
going to be changed again later?".

(In other words: it's true that we can have both, but that has a price
as well).

-- 
Eduardo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]