[PATCH] checkpatch: Add test for #defines of ARCH_HAS_

Joe Perches
2013-09-26T02:01:02Z

On Wed, 2013-09-25 at 18:26 -0700, Linus Torvalds wrote:
> On Wed, Sep 25, 2013 at 5:40 PM, Joe Perches <joe@perches.com> wrote:
> >
> > Huh? That matches all the ARCH_HAS_<foo> patterns.
>
> Right. And they are all crap. lib/string.c is a prime example of
> something that should never have happened.
>
> The ARCH_HAS_xyz pattern is totally retarded. It's wrong.
>
> For big conceptual features, we should use Kconfig symbols.
>
> And for smaller things - like lib/string.c - where we have
> compatibility fallback functions but want architectures able to
> override them with optimized ones one function at a time, we should
> either use weak functions (appropriate for some cases), or the symbol
> that protects them should the the SAME SYMBOL WE USE. Rather than some
> made-up crap-for-brains new ARCH_HAS_xyz symbol. That way it shows up
> in greps, and that way we don't have any question about what random
> symbol pattern we use that particular day.
>
> So for *bad* use, see lib/string.c, and the ARCH_AS_xyz horror.
>
> For *good* use, see lib/div64.c or lib/find_next_bit.c.
>
> Notice how div64.c doesn't make up new ARCH_HAS_random_crap names? And
> no, you don't have to define those things as macros, you can define
> them as functions (inline or not), and then just do
>
> #define find_next_zero_bit find_next_zero_bit
>
> to tell the rest of the world "Look, I have this defined".
>
> The whole "make up a totally unrelated second name for it" means that
> we have things like __HAVE_ARCH_STRLEN but also things like
> ARCH_HAS_PREFETCHW. Ugh.
>
> Linus

So, add a test for these #defines

Additionally, moved string_find_replace sub as it
screws up subsequent formatting when placed inside
another sub.

Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Joe Perches <joe@perches.com>
---
scripts/checkpatch.pl | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c03e427..e2e7703 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1512,6 +1512,14 @@ sub rtrim {
return $string;
}

+sub string_find_replace {
+ my ($string, $find, $replace) = @_;
+
+ $string =~ s/$find/$replace/g;
+
+ return $string;
+}
+
sub tabify {
my ($leading) = @_;

@@ -3731,14 +3739,6 @@ sub process {
}
}

-sub string_find_replace {
- my ($string, $find, $replace) = @_;
-
- $string =~ s/$find/$replace/g;
-
- return $string;
-}
-
# check for bad placement of section $InitAttribute (e.g.: __initdata)
if ($line =~ /(\b$InitAttribute\b)/) {
my $attr = $1;
@@ -4196,6 +4196,12 @@ sub string_find_replace {
"usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc\n" . $herecurr);
}

+# Use of __ARCH_HAS_<FOO> or ARCH_HAVE_<BAR> is wrong.
+ if ($line =~ /\+\s*#\s*define\s+((?:__)?ARCH_(?:HAS|HAVE)\w*)\b/) {
+ ERROR("DEFINE_ARCH_HAS",
+ "#define of '$1' is wrong - use Kconfig variables or standard guards instead\n" . $herecurr);
+ }
+
# check for %L{u,d,i} in strings
my $string;
while ($line =~ /(?:^|")([X\t]*)(?:"|$)/g) {