On Wed, Jan 23, 2013 at 06:32:29PM +0100, Markus Armbruster wrote: > Eduardo Habkost <ehabk...@redhat.com> 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