Re: [PATCH v2] kconfig: autogenerated config_is_xxx macro

[Arnaud Lacombe]

Hi,

On Fri, May 13, 2011 at 4:09 AM, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@xxxxxxxxxxxx> wrote:
> On 10:52 Mon 09 May     , Michal Marek wrote:
>> On 7.5.2011 03:50, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> >On 12:19 Fri 06 May     , Arnaud Lacombe wrote:
>> >>Why would it be a good thing ?
>> >>
>> >>Most configuration-dependent code inside functions tends to be moved
>> >>to a static inline already, which get conditionally defined based on
>> >>the CONFIG_<foo>. If it is not, then the code is badly architectured
>> >>(->  bad). Using that if(xxx) notation would also lead to yet more
>> >>heavily indented function (->  bad). Moreover, this introduces
>> >>yet-another way to check for an information (->  bad), and you will end
>> >>up with mixing the config_is_<xxx>  notation inside a function
>> >>declaration, and CONFIG_<xxx>  when not inside a function (->  bad)
>> >>
>> >>Actually, this is even worse than that as you'll not be able to hide
>> >>structure (or structure members) inside CONFIG_<xxx>  and use that
>> >>structure (or structure members) in config_is_<xxx>  protected block
>> >>without causing compile-time failure.
>> >sorry but conditionnal structure members is bad practice
>> >you save nearly no space nut for the test of the code in multiple
>> >configuration. Use union for this.
>> >
>> >the compile-time failure is good here. it's means your code is not generic.
>> >
>> >specially when you want to keep code running on multiple soc/arch keep compiling
>> >no matter the configuration
>> >
>> >#ifdef in the code is a really bad habit
>>
>> Do you have proof of concept patches that make use of the
>> config_is_xxx macros? Acked by the respective subsystem maintainers?
>> It would be a good idea to send them along to show that this feature
>> is going to be actually used.
> I've seen thousands of place in the kernel we can use
> so I'll just take one example on x86
>
> the patch attached is just an example
>
An you get a nice build error, at least from:

 void pcibios_penalize_isa_irq(int irq, int active)
 {
-#ifdef CONFIG_ACPI
-	if (!acpi_noirq)
+	if (config_is_pci_bios() && !acpi_noirq)
 		acpi_penalize_isa_irq(irq, active);
 	else
-#endif
 		pirq_penalize_isa_irq(irq, active);
 }

as acpi_penalize_isa_irq() is only declared if CONFIG_ACPI is. So be
prepared to fix a lot of code.

I don't really care about the good or the bad, of each solution. These
are just tools, they are not intrinsically good or bad, only their
(over/under)usage might eventually get criticized. To further extend,
I am not sure you can keep x86-64 and x86-32 merged in the same
`arch/x86' tree without using a single #ifdef in struct definition and
function declaration.

 - Arnaud

> Best Regards,
> J.
>
--
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/