Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1759455Ab1EMPTq (ORCPT <rfc822;w@1wt.eu>);
	Fri, 13 May 2011 11:19:46 -0400
Received: from mail-iw0-f174.google.com ([209.85.214.174]:39437 "EHLO
	mail-iw0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1757522Ab1EMPTn convert rfc822-to-8bit (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 13 May 2011 11:19:43 -0400
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=mime-version:in-reply-to:references:date:message-id:subject:from:to
         :cc:content-type:content-transfer-encoding;
        b=MUxFqQPB3Xbo+H4vHqCG8Rw4EWyLbVevgpF9QRKyEg9/LhX6rmYSEL+Jxnu6eEnWP5
         1iAazg1P9Gj8I3YlTbQSYZt+Qu12rgmBuUA6xp530GX9/Zbg8W8t62sahqHbp7r8EXjL
         7UIZWO9SUryuKOfz71iUIFSMtq4HPrAbNoayM=
MIME-Version: 1.0
In-Reply-To: <20110513080909.GO18952@game.jcrosoft.org>
References: <1304658229-30820-1-git-send-email-plagnioj@jcrosoft.com>
	<BANLkTi=s8w01NHnxB7cfCvy4ZS5GnUABZg@mail.gmail.com>
	<20110507015041.GA21017@game.jcrosoft.org>
	<4DC7AB57.9050002@suse.cz>
	<20110513080909.GO18952@game.jcrosoft.org>
Date: Fri, 13 May 2011 11:19:42 -0400
Message-ID: <BANLkTi=t3jCk=A9R0X4mbsCJHHMWNt-fDg@mail.gmail.com>
Subject: Re: [PATCH v2] kconfig: autogenerated config_is_xxx macro
From: Arnaud Lacombe <lacombar@gmail.com>
To: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Michal Marek <mmarek@suse.cz>, linux-kernel@vger.kernel.org,
        linux-kbuild@vger.kernel.org, x86@kernel.org,
        Ingo Molnar <mingo@redhat.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2989
Lines: 75

Hi,

On Fri, May 13, 2011 at 4:09 AM, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@jcrosoft.com> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/