All of lore.kernel.org
 help / color / mirror / Atom feed
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Olof Johansson <olof@austin.ibm.com>
Cc: Andrew Morton <akpm@osdl.org>, Pekka Enberg <penberg@gmail.com>,
	Tom Rini <trini@kernel.crashing.org>,
	Linux Kernel list <linux-kernel@vger.kernel.org>,
	linuxppc-dev list <linuxppc-dev@ozlabs.org>,
	penberg@cs.helsinki.fi, Paul Mackerras <paulus@samba.org>,
	"H. Peter Anvin" <hpa@zytor.com>,
	linuxppc64-dev <linuxppc64-dev@ozlabs.org>
Subject: Re: [PATCH] PPC/PPC64: Introduce CPU_HAS_FEATURE() macro
Date: Sat, 05 Feb 2005 20:08:53 +1100	[thread overview]
Message-ID: <1107594534.30270.3.camel@gaston> (raw)
In-Reply-To: <20050204172041.GA17586@austin.ibm.com>

On Fri, 2005-02-04 at 11:20 -0600, Olof Johansson wrote:
> On Fri, Feb 04, 2005 at 10:17:48AM +0200, Pekka Enberg wrote:
> > Please drop the CPU_FTR_##x macro magic as it makes grepping more
> > complicated. If the enum names are too long, just do s/CPU_FTR_/CPU_/g
> > or something similar. Also, could you please make this a static inline
> > function?

I tend to agree with Pekka...

> I considered that for a while, but decided against it because:
> 
> * cpu-has-feature(cpu-feature-foo) v cpu-has-feature(foo): I picked the
> latter for readability.

I don't think it really matters compared to the usefullness of grep, and
is still more readable than the old way...

> * Renaming CPU_FTR_<x> -> CPU_<x> makes it less obvious that
> it's actually a cpu feature it's describing (i.e. CPU_ALTIVEC vs
> CPU_FTR_ALTIVEC).

Agreed.

> * Renaming would clobber the namespace, CPU_* definitions are used in
> other places in the tree.
> * Can't make it an inline and still use the preprocessor concatenation.

I'd like to keep the constants as-is and have the stuff inline with no
macro trick as Pekka suggest since I did use grep on those things quite
often.

> That being said, you do have a point about grepability. However,
> personally I'd be more likely to look for CPU_HAS_FEATURE than the
> feature itself when reading the code, and would find that easily. The
> other way around (finding all uses of a feature) is harder, but the
> concatenation macro is right below the bit definitions and easy to spot.

No, when I grep, i'm looking for the feature itself...

Ben.

WARNING: multiple messages have this Message-ID (diff)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Olof Johansson <olof@austin.ibm.com>
Cc: Pekka Enberg <penberg@gmail.com>,
	linuxppc64-dev <linuxppc64-dev@ozlabs.org>,
	linuxppc-dev list <linuxppc-dev@ozlabs.org>,
	Linux Kernel list <linux-kernel@vger.kernel.org>,
	Paul Mackerras <paulus@samba.org>,
	Anton Blanchard <anton@samba.org>,
	Tom Rini <trini@kernel.crashing.org>,
	"H. Peter Anvin" <hpa@zytor.com>, Andrew Morton <akpm@osdl.org>,
	penberg@cs.helsinki.fi
Subject: Re: [PATCH] PPC/PPC64: Introduce CPU_HAS_FEATURE() macro
Date: Sat, 05 Feb 2005 20:08:53 +1100	[thread overview]
Message-ID: <1107594534.30270.3.camel@gaston> (raw)
In-Reply-To: <20050204172041.GA17586@austin.ibm.com>

On Fri, 2005-02-04 at 11:20 -0600, Olof Johansson wrote:
> On Fri, Feb 04, 2005 at 10:17:48AM +0200, Pekka Enberg wrote:
> > Please drop the CPU_FTR_##x macro magic as it makes grepping more
> > complicated. If the enum names are too long, just do s/CPU_FTR_/CPU_/g
> > or something similar. Also, could you please make this a static inline
> > function?

I tend to agree with Pekka...

> I considered that for a while, but decided against it because:
> 
> * cpu-has-feature(cpu-feature-foo) v cpu-has-feature(foo): I picked the
> latter for readability.

I don't think it really matters compared to the usefullness of grep, and
is still more readable than the old way...

> * Renaming CPU_FTR_<x> -> CPU_<x> makes it less obvious that
> it's actually a cpu feature it's describing (i.e. CPU_ALTIVEC vs
> CPU_FTR_ALTIVEC).

Agreed.

> * Renaming would clobber the namespace, CPU_* definitions are used in
> other places in the tree.
> * Can't make it an inline and still use the preprocessor concatenation.

I'd like to keep the constants as-is and have the stuff inline with no
macro trick as Pekka suggest since I did use grep on those things quite
often.

> That being said, you do have a point about grepability. However,
> personally I'd be more likely to look for CPU_HAS_FEATURE than the
> feature itself when reading the code, and would find that easily. The
> other way around (finding all uses of a feature) is harder, but the
> concatenation macro is right below the bit definitions and easy to spot.

No, when I grep, i'm looking for the feature itself...

Ben.



  parent reply	other threads:[~2005-02-05  9:08 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2005-02-04  7:22 [PATCH] PPC/PPC64: Introduce CPU_HAS_FEATURE() macro Olof Johansson
2005-02-04  7:22 ` Olof Johansson
2005-02-04  8:17 ` Pekka Enberg
2005-02-04  8:17   ` Pekka Enberg
2005-02-04 17:20   ` Olof Johansson
2005-02-04 17:20     ` Olof Johansson
2005-02-05  7:48     ` Pekka Enberg
2005-02-05  7:48       ` Pekka Enberg
2005-02-05  9:08     ` Benjamin Herrenschmidt [this message]
2005-02-05  9:08       ` Benjamin Herrenschmidt
2005-02-04 12:36 ` Arnd Bergmann
2005-02-04 12:36   ` Arnd Bergmann
2005-02-04 18:35   ` Olof Johansson
2005-02-04 18:35     ` Olof Johansson
2005-02-04 18:57     ` Arnd Bergmann
2005-02-04 18:57       ` Arnd Bergmann
2005-02-04 23:50     ` Benjamin Herrenschmidt
2005-02-04 23:50       ` Benjamin Herrenschmidt
2005-02-04 23:49   ` Benjamin Herrenschmidt
2005-02-04 23:49     ` Benjamin Herrenschmidt
2005-02-05  0:22     ` Arnd Bergmann
2005-02-05  0:22       ` Arnd Bergmann
2005-02-05  1:34       ` Anton Blanchard
2005-02-05  1:34         ` Anton Blanchard
2005-02-05 11:04         ` Arnd Bergmann
2005-02-05 11:04           ` Arnd Bergmann
2005-02-05  1:47       ` Benjamin Herrenschmidt
2005-02-05  1:47         ` Benjamin Herrenschmidt
2005-02-04 14:45 ` Tom Rini
2005-02-04 14:45   ` Tom Rini
2005-02-05 18:46 ` [PATCH] PPC/PPC64: Abstract cpu_feature checks Olof Johansson
2005-02-05 18:46   ` Olof Johansson
2005-02-06  3:26   ` Olof Johansson
2005-02-06  3:26     ` Olof Johansson
2005-02-06 11:57     ` Arnd Bergmann
2005-02-06 11:57       ` Arnd Bergmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1107594534.30270.3.camel@gaston \
    --to=benh@kernel.crashing.org \
    --cc=akpm@osdl.org \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=linuxppc64-dev@ozlabs.org \
    --cc=olof@austin.ibm.com \
    --cc=paulus@samba.org \
    --cc=penberg@cs.helsinki.fi \
    --cc=penberg@gmail.com \
    --cc=trini@kernel.crashing.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.