Re: [patch 3/3] x86: io-apic - code style cleaning forsetup_IO_APIC_irqs

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@xxxxxxxxx> wrote:

> [Ingo Molnar - Fri, Sep 05, 2008 at 10:04:47AM +0200]
> | 
> | * Cyrill Gorcunov <gorcunov@xxxxxxxxx> wrote:
> | 
> | > Use a nested level for 'for' cycle and break long lines.
> | > For apic_print we should countinue using KERNEL_DEBUG if
> | > we have started to.
> | 
> | > @@ -1521,32 +1521,35 @@ static void __init setup_IO_APIC_irqs(vo
> | >  	apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
> | >  
> | >  	for (apic = 0; apic < nr_ioapics; apic++) {
> | > -	for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
> | > +		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
> | >  
> | > +			idx = find_irq_entry(apic, pin, mp_INT);
> | > +			if (idx == -1) {
> | 
> | hm, i dont really like the super-deep nesting we do here. Could you 
> | please split out the iterator into a separate function? That makes the 
> | code a lot easier to understand and saves two extra tabs as well for 
> | those ugly-looking printk lines.
> | 
> | 	Ingo
> | 
> 
> You know it seems we use such a 'cycle on every pin on io-apics'
> in several places for now:
> 
> io_apic.c
> ---------
> 	clear_IO_APIC
> 	save_mask_IO_APIC_setup
> 	restore_IO_APIC_setup
> 	IO_APIC_irq_trigger
> 	setup_IO_APIC_irqs
> 
> I've made a one-line macro for this (like for_all_ioapics_pins)
> _but_ it looks much more ugly then this two nested for(;;) :)
> 
> If you meant me to make a separate iterator over the pins I think
> it will not help a lot - this function is simple enought so the only
> problem is too-long-printk-form - maybe just print them on separated
> lines instead of tracking apicids? Or it was made in a sake to not
> scroll screen too much?

hm, by iterator i meant the body itself. I.e. something like:

static void __init setup_IO_APIC_irqs(void)
{
        int apic, pin, notcon = 1;

        apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");

        for (apic = 0; apic < nr_ioapics; apic++)
		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++)
			notcon = setup_ioapic_irq(apic, pin, notcon);

        if (!notcon)
                apic_printk(APIC_VERBOSE, " not connected.\n");
}

this looks quite a bit cleaner, doesnt it? We lose the 'idx' and 'irq' 
variables and we lose the curly braces as well. The flow looks a lot 
more trivial. And the new setup_ioapic_irq() function will be simpler as 
well - it will only have 'idx' and 'irq' as a local variable, the rest 
comes in as a parameter. It can 'return notcon' instead of 'continue'. 
And it will be 2 levels of tabs aligned to the left, as an added bonus.

Hm?

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/