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-kbuild" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
