From: SeongJae Park <[email protected]>
Some terms could be deprecated for various reasons, but it is hard to
update the entire old usages. That said, we could at least encourage
new patches to use the suggested replacements. This commit adds check
of deprecated terms in the 'checkpatch.pl' for that. The script will
get deprecated terms and suggested replacements of those from
'scripts/deprecated_terms.txt' file and warn if the deprecated terms are
used. The mechanism and the format of the file are almost the same as
that of 'spelling.txt'. For the reason, this commit modularizes the
read of the 'spelling.txt' and reuses.
Signed-off-by: SeongJae Park <[email protected]>
---
scripts/checkpatch.pl | 60 +++++++++++++++++++++++++++---------
scripts/deprecated_terms.txt | 5 +++
2 files changed, 50 insertions(+), 15 deletions(-)
create mode 100644 scripts/deprecated_terms.txt
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 524df88f9364..c672091932bb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -57,6 +57,7 @@ my $max_line_length = 100;
my $ignore_perl_version = 0;
my $minimum_perl_version = 5.10.0;
my $min_conf_desc_length = 4;
+my $deprecated_terms_file = "$D/deprecated_terms.txt";
my $spelling_file = "$D/spelling.txt";
my $codespell = 0;
my $codespellfile = "/usr/share/codespell/dictionary.txt";
@@ -692,29 +693,40 @@ our $allowed_asm_includes = qr{(?x:
)};
# memory.h: ARM has a custom one
-# Load common spelling mistakes and build regular expression list.
-my $misspellings;
-my %spelling_fix;
+sub read_word_corrections {
+ my ($file, $fixesRef) = @_;
+ my $suspects;
-if (open(my $spelling, '<', $spelling_file)) {
- while (<$spelling>) {
- my $line = $_;
+ if (open(my $corrections, '<', $file)) {
+ while (<$corrections>) {
+ my $line = $_;
- $line =~ s/\s*\n?$//g;
- $line =~ s/^\s*//g;
+ $line =~ s/\s*\n?$//g;
+ $line =~ s/^\s*//g;
- next if ($line =~ m/^\s*#/);
- next if ($line =~ m/^\s*$/);
+ next if ($line =~ m/^\s*#/);
+ next if ($line =~ m/^\s*$/);
- my ($suspect, $fix) = split(/\|\|/, $line);
+ my ($suspect, $fix) = split(/\|\|/, $line);
- $spelling_fix{$suspect} = $fix;
+ $fixesRef->{$suspect} = $fix;
+ }
+ close($corrections);
+ } else {
+ warn "No correction will be found - file '$file': $!\n";
}
- close($spelling);
-} else {
- warn "No typos will be found - file '$spelling_file': $!\n";
}
+# Load deprecated terms and build regular expression list.
+my %deprecated_terms_fix;
+read_word_corrections($deprecated_terms_file, \%deprecated_terms_fix);
+my $deprecated_terms = join("|", sort keys %deprecated_terms_fix) if keys %deprecated_terms_fix;
+
+# Load common spelling mistakes and build regular expression list.
+my $misspellings;
+my %spelling_fix;
+read_word_corrections($spelling_file, \%spelling_fix);
+
if ($codespell) {
if (open(my $spelling, '<', $codespellfile)) {
while (<$spelling>) {
@@ -2957,6 +2969,24 @@ sub process {
}
}
+# Check for deprecated terms
+ if (defined($deprecated_terms) &&
+ ($in_commit_log || $line =~ /^(?:\+|Subject:)/i)) {
+ while ($rawline =~ /(?:^|[^a-z@])($deprecated_terms)(?:\b|$|[^a-z@])/gi) {
+ my $deprecated_term = $1;
+ my $suggested = $deprecated_terms_fix{lc($deprecated_term)};
+ $suggested = ucfirst($suggested) if ($deprecated_term=~ /^[A-Z]/);
+ $suggested = uc($suggested) if ($deprecated_term =~ /^[A-Z]+$/);
+ my $msg_level = \&WARN;
+ $msg_level = \&CHK if ($file);
+ if (&{$msg_level}("DEPRECATED_TERM",
+ "Use of '$deprecated_term' is deprecated, please '$suggested', instead.\n" . $herecurr) &&
+ $fix) {
+ $fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($deprecated_term)($|[^A-Za-z@])/$1$suggested$3/;
+ }
+ }
+ }
+
# Check for various typo / spelling mistakes
if (defined($misspellings) &&
($in_commit_log || $line =~ /^(?:\+|Subject:)/i)) {
diff --git a/scripts/deprecated_terms.txt b/scripts/deprecated_terms.txt
new file mode 100644
index 000000000000..6faa06451c3d
--- /dev/null
+++ b/scripts/deprecated_terms.txt
@@ -0,0 +1,5 @@
+# License: GPLv2
+#
+# The format of each line is:
+# deprecated||suggested
+#
--
2.17.1
Hello,
I see that this patch went into next and is already inciting people to
do wrong things [1]. Can you please fix it to require '--subjective'
switch or otherwise mark it clearly as suggestion-only?
The coding-style as in Linus' master says about *NEW* uses of the words
listed (those introductions I expect to be actually rare) and not about
existing use in the code or industry. Making a noise about all uses
found surely will generate a lot more irrelevant patches.
[1] https://www.spinics.net/lists/linux-tegra/msg51849.html
Best Regards
Micha??Miros?aw
On Sat, 2020-07-25 at 15:02 +0200, Michał Mirosław wrote:
> Hello,
>
> I see that this patch went into next and is already inciting people to
> do wrong things [1]. Can you please fix it to require '--subjective'
> switch or otherwise mark it clearly as suggestion-only?
>
> The coding-style as in Linus' master says about *NEW* uses of the words
> listed (those introductions I expect to be actually rare) and not about
> existing use in the code or industry. Making a noise about all uses
> found surely will generate a lot more irrelevant patches.
>
> [1] https://www.spinics.net/lists/linux-tegra/msg51849.html
I was never a big fan of this change.
Andrew, can you revert this please?
On Sat, 2020-07-25 at 15:02 +0200, Michał Mirosław wrote:
> Hello,
>
> I see that this patch went into next and is already inciting people to
> do wrong things [1]. Can you please fix it to require '--subjective'
> switch or otherwise mark it clearly as suggestion-only?
>
> The coding-style as in Linus' master says about *NEW* uses of the words
> listed (those introductions I expect to be actually rare) and not about
> existing use in the code or industry. Making a noise about all uses
> found surely will generate a lot more irrelevant patches.
>
> [1] https://www.spinics.net/lists/linux-tegra/msg51849.html
And if not reverted, perhaps do not check existing files
at all but only check patches and change the message to
show only suggestions not from a specification.
---
scripts/checkpatch.pl | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e9fde28eb0de..7ef1ba80cb20 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2970,21 +2970,16 @@ sub process {
}
}
-# Check for deprecated terms
- if (defined($deprecated_terms) &&
+# Check for deprecated terms not used by a specification (not used on files)
+ if (!$file && defined($deprecated_terms) &&
($in_commit_log || $line =~ /^(?:\+|Subject:)/i)) {
while ($rawline =~ /(?:^|[^a-z@])($deprecated_terms)(?:\b|$|[^a-z@])/gi) {
- my $deprecated_term = $1;
- my $suggested = $deprecated_terms_fix{lc($deprecated_term)};
- $suggested = ucfirst($suggested) if ($deprecated_term=~ /^[A-Z]/);
- $suggested = uc($suggested) if ($deprecated_term =~ /^[A-Z]+$/);
- my $msg_level = \&WARN;
- $msg_level = \&CHK if ($file);
- if (&{$msg_level}("DEPRECATED_TERM",
- "Use of '$deprecated_term' is deprecated, please '$suggested', instead.\n" . $herecurr) &&
- $fix) {
- $fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($deprecated_term)($|[^A-Za-z@])/$1$suggested$3/;
- }
+ my $deprecate = $1;
+ my $suggest = $deprecated_terms_fix{lc($deprecate)};
+ $suggest = ucfirst($suggest) if ($deprecate =~ /^[A-Z]/);
+ $suggest = uc($suggest) if ($deprecate =~ /^[A-Z]+$/);
+ CHK("DEPRECATED_TERM",
+ "Use of '$deprecate' is controversial - if not required by specification, perhaps '$suggest' instead\n" . $herecurr);
}
}
On Sat, 25 Jul 2020 10:29:23 -0700 Joe Perches <[email protected]> wrote:
> On Sat, 2020-07-25 at 15:02 +0200, Michał Mirosław wrote:
> > Hello,
> >
> > I see that this patch went into next and is already inciting people to
> > do wrong things [1]. Can you please fix it to require '--subjective'
> > switch or otherwise mark it clearly as suggestion-only?
> >
> > The coding-style as in Linus' master says about *NEW* uses of the words
> > listed (those introductions I expect to be actually rare) and not about
> > existing use in the code or industry. Making a noise about all uses
> > found surely will generate a lot more irrelevant patches.
> >
> > [1] https://www.spinics.net/lists/linux-tegra/msg51849.html
>
> And if not reverted, perhaps do not check existing files
> at all but only check patches and change the message to
> show only suggestions not from a specification.
Agreed for this case. However, excluding existing file check doesn't fully
avoid this problem. Also, more terms having different deprecation rules might
be added in future. How about allowing file check but show reference in the
suggestion message as below?
> ---
[...]
Thanks,
SeongJae Park
================================ >8 ===========================================
From aeb852296bc40ca1de8a6a11f4d5368b02d2e417 Mon Sep 17 00:00:00 2001
From: SeongJae Park <[email protected]>
Date: Sun, 26 Jul 2020 01:14:48 +0200
Subject: [PATCH] scripts/deprecatd_terms: provide references
Deprecation of terms could have special rules. For example, 'slave' is
ok for existing usages. Same to 'master', but it's also ok unless it's
used with 'slave'. This commit provides the references for such rules.
Signed-off-by: SeongJae Park <[email protected]>
---
scripts/checkpatch.pl | 2 +-
scripts/deprecated_terms.txt | 6 ++++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e9fde28eb0de..77f5f777b053 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2981,7 +2981,7 @@ sub process {
my $msg_level = \&WARN;
$msg_level = \&CHK if ($file);
if (&{$msg_level}("DEPRECATED_TERM",
- "Use of '$deprecated_term' is deprecated, please '$suggested', instead.\n" . $herecurr) &&
+ "Use of '$deprecated_term' is controversial - if not required by specification, perhaps '$suggested' instead. See: scripts/deprecated_terms.txt\n" . $herecurr) &&
$fix) {
$fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($deprecated_term)($|[^A-Za-z@])/$1$suggested$3/;
}
diff --git a/scripts/deprecated_terms.txt b/scripts/deprecated_terms.txt
index 1be27a24187b..d92b9c896fce 100644
--- a/scripts/deprecated_terms.txt
+++ b/scripts/deprecated_terms.txt
@@ -3,8 +3,10 @@
# The format of each line is:
# deprecated||suggested
#
+# If special rules are applied on the terms, please comment those.
+#
+# Refer to "4) Naming" section of Documentation/process/coding-style.rst for
+# below three terms.
blacklist||(denylist|blocklist)
-# For other alternatives of 'slave', Please refer to
-# Documentation/process/coding-style.rst
slave||(secondary|target|...)
whitelist||(allowlist|passlist)
--
2.17.1
On Sun, 2020-07-26 at 01:35 +0200, SeongJae Park wrote:
> On Sat, 25 Jul 2020 10:29:23 -0700 Joe Perches <[email protected]> wrote:
>
> > On Sat, 2020-07-25 at 15:02 +0200, Michał Mirosław wrote:
> > > Hello,
> > >
> > > I see that this patch went into next and is already inciting people to
> > > do wrong things [1]. Can you please fix it to require '--subjective'
> > > switch or otherwise mark it clearly as suggestion-only?
> > >
> > > The coding-style as in Linus' master says about *NEW* uses of the words
> > > listed (those introductions I expect to be actually rare) and not about
> > > existing use in the code or industry. Making a noise about all uses
> > > found surely will generate a lot more irrelevant patches.
> > >
> > > [1] https://www.spinics.net/lists/linux-tegra/msg51849.html
> >
> > And if not reverted, perhaps do not check existing files
> > at all but only check patches and change the message to
> > show only suggestions not from a specification.
>
> Agreed for this case. However, excluding existing file check doesn't fully
> avoid this problem. Also, more terms having different deprecation rules might
> be added in future. How about allowing file check but show reference in the
> suggestion message as below?
The general problem is that drivers/staging, net/ and drivers/net
all have --strict on by default.
Emitting these deprecated terms messages with -f --file uses for
files in those directories isn't a great idea.
> diff --git a/scripts/deprecated_terms.txt b/scripts/deprecated_terms.txt
[]
> @@ -3,8 +3,10 @@
> # The format of each line is:
> # deprecated||suggested
> #
> +# If special rules are applied on the terms, please comment those.
Disagree. Comments about these existing uses aren't helpful.
> +#
> +# Refer to "4) Naming" section of Documentation/process/coding-style.rst for
> +# below three terms.
> blacklist||(denylist|blocklist)
> -# For other alternatives of 'slave', Please refer to
> -# Documentation/process/coding-style.rst
> slave||(secondary|target|...)
> whitelist||(allowlist|passlist)
On Sat, 25 Jul 2020 21:27:07 -0700 Joe Perches <[email protected]> wrote:
> On Sun, 2020-07-26 at 01:35 +0200, SeongJae Park wrote:
> > On Sat, 25 Jul 2020 10:29:23 -0700 Joe Perches <[email protected]> wrote:
> >
> > > On Sat, 2020-07-25 at 15:02 +0200, Michał Mirosław wrote:
> > > > Hello,
> > > >
> > > > I see that this patch went into next and is already inciting people to
> > > > do wrong things [1]. Can you please fix it to require '--subjective'
> > > > switch or otherwise mark it clearly as suggestion-only?
> > > >
> > > > The coding-style as in Linus' master says about *NEW* uses of the words
> > > > listed (those introductions I expect to be actually rare) and not about
> > > > existing use in the code or industry. Making a noise about all uses
> > > > found surely will generate a lot more irrelevant patches.
> > > >
> > > > [1] https://www.spinics.net/lists/linux-tegra/msg51849.html
> > >
> > > And if not reverted, perhaps do not check existing files
> > > at all but only check patches and change the message to
> > > show only suggestions not from a specification.
> >
> > Agreed for this case. However, excluding existing file check doesn't fully
> > avoid this problem. Also, more terms having different deprecation rules might
> > be added in future. How about allowing file check but show reference in the
> > suggestion message as below?
>
> The general problem is that drivers/staging, net/ and drivers/net
> all have --strict on by default.
>
> Emitting these deprecated terms messages with -f --file uses for
> files in those directories isn't a great idea.
Thank you for kindly explaining your concenrs in detail. However, I think it's
ok to do this check even without '--strict' for files if we explicitly says
it's suggestion only, as Michal said. My patch does so.
>
> > diff --git a/scripts/deprecated_terms.txt b/scripts/deprecated_terms.txt
> []
> > @@ -3,8 +3,10 @@
> > # The format of each line is:
> > # deprecated||suggested
> > #
> > +# If special rules are applied on the terms, please comment those.
>
> Disagree. Comments about these existing uses aren't helpful.
Sorry, I don't understand your point here. Why do you think it's not helpful?
If 'checkpatch' finds the deprecated terms, it will ask people to read this
file, which explains special rules for each of the deprecations if exists. The
rule is, in the case of 'slave', 'applies to new uses only'. Therefore, people
could stop sending the noisy unnecessary patches to the maintainers.
Thanks,
SeongJae Park
>
> > +#
> > +# Refer to "4) Naming" section of Documentation/process/coding-style.rst for
> > +# below three terms.
> > blacklist||(denylist|blocklist)
> > -# For other alternatives of 'slave', Please refer to
> > -# Documentation/process/coding-style.rst
> > slave||(secondary|target|...)
> > whitelist||(allowlist|passlist)
>
On Sun, 2020-07-26 at 09:18 +0200, SeongJae Park wrote:
> On Sat, 25 Jul 2020 21:27:07 -0700 Joe Perches <[email protected]> wrote:
>
> > On Sun, 2020-07-26 at 01:35 +0200, SeongJae Park wrote:
> > > On Sat, 25 Jul 2020 10:29:23 -0700 Joe Perches <[email protected]> wrote:
> > >
> > > > On Sat, 2020-07-25 at 15:02 +0200, Michał Mirosław wrote:
> > > > > Hello,
> > > > >
> > > > > I see that this patch went into next and is already inciting people to
> > > > > do wrong things [1]. Can you please fix it to require '--subjective'
> > > > > switch or otherwise mark it clearly as suggestion-only?
> > > > >
> > > > > The coding-style as in Linus' master says about *NEW* uses of the words
> > > > > listed (those introductions I expect to be actually rare) and not about
> > > > > existing use in the code or industry. Making a noise about all uses
> > > > > found surely will generate a lot more irrelevant patches.
> > > > >
> > > > > [1] https://www.spinics.net/lists/linux-tegra/msg51849.html
> > > >
> > > > And if not reverted, perhaps do not check existing files
> > > > at all but only check patches and change the message to
> > > > show only suggestions not from a specification.
> > >
> > > Agreed for this case. However, excluding existing file check doesn't fully
> > > avoid this problem. Also, more terms having different deprecation rules might
> > > be added in future. How about allowing file check but show reference in the
> > > suggestion message as below?
> >
> > The general problem is that drivers/staging, net/ and drivers/net
> > all have --strict on by default.
> >
> > Emitting these deprecated terms messages with -f --file uses for
> > files in those directories isn't a great idea.
>
> Thank you for kindly explaining your concenrs in detail. However, I think it's
> ok to do this check even without '--strict' for files if we explicitly says
> it's suggestion only, as Michal said. My patch does so.
>
> > > diff --git a/scripts/deprecated_terms.txt b/scripts/deprecated_terms.txt
> > []
> > > @@ -3,8 +3,10 @@
> > > # The format of each line is:
> > > # deprecated||suggested
> > > #
> > > +# If special rules are applied on the terms, please comment those.
> >
> > Disagree. Comments about these existing uses aren't helpful.
>
> Sorry, I don't understand your point here. Why do you think it's not helpful?
> If 'checkpatch' finds the deprecated terms, it will ask people to read this
> file, which explains special rules for each of the deprecations if exists. The
> rule is, in the case of 'slave', 'applies to new uses only'. Therefore, people
> could stop sending the noisy unnecessary patches to the maintainers.
Because it will describe this for _every_ instance
of any deprecated word in the file.
On Sun, 26 Jul 2020 00:29:05 -0700 Joe Perches <[email protected]> wrote:
> On Sun, 2020-07-26 at 09:18 +0200, SeongJae Park wrote:
> > On Sat, 25 Jul 2020 21:27:07 -0700 Joe Perches <[email protected]> wrote:
> >
> > > On Sun, 2020-07-26 at 01:35 +0200, SeongJae Park wrote:
> > > > On Sat, 25 Jul 2020 10:29:23 -0700 Joe Perches <[email protected]> wrote:
> > > >
> > > > > On Sat, 2020-07-25 at 15:02 +0200, Michał Mirosław wrote:
> > > > > > Hello,
> > > > > >
> > > > > > I see that this patch went into next and is already inciting people to
> > > > > > do wrong things [1]. Can you please fix it to require '--subjective'
> > > > > > switch or otherwise mark it clearly as suggestion-only?
> > > > > >
> > > > > > The coding-style as in Linus' master says about *NEW* uses of the words
> > > > > > listed (those introductions I expect to be actually rare) and not about
> > > > > > existing use in the code or industry. Making a noise about all uses
> > > > > > found surely will generate a lot more irrelevant patches.
> > > > > >
> > > > > > [1] https://www.spinics.net/lists/linux-tegra/msg51849.html
> > > > >
> > > > > And if not reverted, perhaps do not check existing files
> > > > > at all but only check patches and change the message to
> > > > > show only suggestions not from a specification.
> > > >
> > > > Agreed for this case. However, excluding existing file check doesn't fully
> > > > avoid this problem. Also, more terms having different deprecation rules might
> > > > be added in future. How about allowing file check but show reference in the
> > > > suggestion message as below?
> > >
> > > The general problem is that drivers/staging, net/ and drivers/net
> > > all have --strict on by default.
> > >
> > > Emitting these deprecated terms messages with -f --file uses for
> > > files in those directories isn't a great idea.
> >
> > Thank you for kindly explaining your concenrs in detail. However, I think it's
> > ok to do this check even without '--strict' for files if we explicitly says
> > it's suggestion only, as Michal said. My patch does so.
> >
> > > > diff --git a/scripts/deprecated_terms.txt b/scripts/deprecated_terms.txt
> > > []
> > > > @@ -3,8 +3,10 @@
> > > > # The format of each line is:
> > > > # deprecated||suggested
> > > > #
> > > > +# If special rules are applied on the terms, please comment those.
> > >
> > > Disagree. Comments about these existing uses aren't helpful.
> >
> > Sorry, I don't understand your point here. Why do you think it's not helpful?
> > If 'checkpatch' finds the deprecated terms, it will ask people to read this
> > file, which explains special rules for each of the deprecations if exists. The
> > rule is, in the case of 'slave', 'applies to new uses only'. Therefore, people
> > could stop sending the noisy unnecessary patches to the maintainers.
>
> Because it will describe this for _every_ instance
> of any deprecated word in the file.
Thank you for kindly explaining your concern. I personally thought the verbose
warning is not a real problem. Anyway, how about below patch, then? It will
show only one warning or check for each of the terms.
================================= >8 ==========================================
From 6c606c62ea25933db8bb0afec083b5b4b8b3f11f Mon Sep 17 00:00:00 2001
From: SeongJae Park <[email protected]>
Date: Sun, 26 Jul 2020 01:14:48 +0200
Subject: [PATCH] scripts/deprecatd_terms: provide references
Deprecation of terms could have special rules. For example, 'slave' is
ok for existing usages. Same to 'master', but it's also ok unless it's
used with 'slave'. This commit provides the references for such rules.
Also, because the report became more verbose a little, this commit makes
the report to be made for only one instance of each deprecated term.
Signed-off-by: SeongJae Park <[email protected]>
---
scripts/checkpatch.pl | 6 +++++-
scripts/deprecated_terms.txt | 6 ++++--
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e9fde28eb0de..227e088bfe56 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -721,6 +721,7 @@ sub read_word_corrections {
my %deprecated_terms_fix;
read_word_corrections($deprecated_terms_file, \%deprecated_terms_fix);
my $deprecated_terms = join("|", sort keys %deprecated_terms_fix) if keys %deprecated_terms_fix;
+my %deprecated_terms_reported = map { $_ => 1 }
# Load common spelling mistakes and build regular expression list.
my $misspellings;
@@ -2975,13 +2976,16 @@ sub process {
($in_commit_log || $line =~ /^(?:\+|Subject:)/i)) {
while ($rawline =~ /(?:^|[^a-z@])($deprecated_terms)(?:\b|$|[^a-z@])/gi) {
my $deprecated_term = $1;
+ last if (exists($deprecated_terms_reported{$deprecated_term}));
+ $deprecated_terms_reported{$deprecated_term} = 1;
+
my $suggested = $deprecated_terms_fix{lc($deprecated_term)};
$suggested = ucfirst($suggested) if ($deprecated_term=~ /^[A-Z]/);
$suggested = uc($suggested) if ($deprecated_term =~ /^[A-Z]+$/);
my $msg_level = \&WARN;
$msg_level = \&CHK if ($file);
if (&{$msg_level}("DEPRECATED_TERM",
- "Use of '$deprecated_term' is deprecated, please '$suggested', instead.\n" . $herecurr) &&
+ "Use of '$deprecated_term' is controversial - if not required by specification, perhaps '$suggested' instead. See: scripts/deprecated_terms.txt\n" . $herecurr) &&
$fix) {
$fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($deprecated_term)($|[^A-Za-z@])/$1$suggested$3/;
}
diff --git a/scripts/deprecated_terms.txt b/scripts/deprecated_terms.txt
index 1be27a24187b..d92b9c896fce 100644
--- a/scripts/deprecated_terms.txt
+++ b/scripts/deprecated_terms.txt
@@ -3,8 +3,10 @@
# The format of each line is:
# deprecated||suggested
#
+# If special rules are applied on the terms, please comment those.
+#
+# Refer to "4) Naming" section of Documentation/process/coding-style.rst for
+# below three terms.
blacklist||(denylist|blocklist)
-# For other alternatives of 'slave', Please refer to
-# Documentation/process/coding-style.rst
slave||(secondary|target|...)
whitelist||(allowlist|passlist)
--
2.17.1
On Sun, 2020-07-26 at 09:45 +0200, SeongJae Park wrote:
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -721,6 +721,7 @@ sub read_word_corrections {
> my %deprecated_terms_fix;
> read_word_corrections($deprecated_terms_file, \%deprecated_terms_fix);
> my $deprecated_terms = join("|", sort keys %deprecated_terms_fix) if keys %deprecated_terms_fix;
> +my %deprecated_terms_reported = map { $_ => 1 }
overly verbose naming and this doesn't need initialization here.
> @@ -2975,13 +2976,16 @@ sub process {
> ($in_commit_log || $line =~ /^(?:\+|Subject:)/i)) {
> while ($rawline =~ /(?:^|[^a-z@])($deprecated_terms)(?:\b|$|[^a-z@])/gi) {
> my $deprecated_term = $1;
> + last if (exists($deprecated_terms_reported{$deprecated_term}));
next if (...) to check if multiple terms exists on the same line
> + $deprecated_terms_reported{$deprecated_term} = 1;
> +
But this does need to be reset to empty when checking the next file
> my $suggested = $deprecated_terms_fix{lc($deprecated_term)};
> $suggested = ucfirst($suggested) if ($deprecated_term=~ /^[A-Z]/);
> $suggested = uc($suggested) if ($deprecated_term =~ /^[A-Z]+$/);
> my $msg_level = \&WARN;
> $msg_level = \&CHK if ($file);
> if (&{$msg_level}("DEPRECATED_TERM",
> - "Use of '$deprecated_term' is deprecated, please '$suggested', instead.\n" . $herecurr) &&
> + "Use of '$deprecated_term' is controversial - if not required by specification, perhaps '$suggested' instead. See: scripts/deprecated_terms.txt\n" . $herecurr) &&
> $fix) {
> $fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($deprecated_term)($|[^A-Za-z@])/$1$suggested$3/;
I think it simpler to avoid emitting this on existing files.
I do not want to encourage relatively inexperienced people
to run checkpatch and submit inappropriate patches.
On Sun, 26 Jul 2020 07:50:54 -0700 Joe Perches <[email protected]> wrote:
> On Sun, 2020-07-26 at 09:45 +0200, SeongJae Park wrote:
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -721,6 +721,7 @@ sub read_word_corrections {
> > my %deprecated_terms_fix;
> > read_word_corrections($deprecated_terms_file, \%deprecated_terms_fix);
> > my $deprecated_terms = join("|", sort keys %deprecated_terms_fix) if keys %deprecated_terms_fix;
> > +my %deprecated_terms_reported = map { $_ => 1 }
>
> overly verbose naming and this doesn't need initialization here.
>
> > @@ -2975,13 +2976,16 @@ sub process {
> > ($in_commit_log || $line =~ /^(?:\+|Subject:)/i)) {
> > while ($rawline =~ /(?:^|[^a-z@])($deprecated_terms)(?:\b|$|[^a-z@])/gi) {
> > my $deprecated_term = $1;
> > + last if (exists($deprecated_terms_reported{$deprecated_term}));
>
> next if (...) to check if multiple terms exists on the same line
Agreed on these comments, thanks!
>
> > + $deprecated_terms_reported{$deprecated_term} = 1;
> > +
>
> But this does need to be reset to empty when checking the next file
Hmm... I though you mean reporting same term multiple times too verbose... Did
I misunderstand your point?
>
> > my $suggested = $deprecated_terms_fix{lc($deprecated_term)};
> > $suggested = ucfirst($suggested) if ($deprecated_term=~ /^[A-Z]/);
> > $suggested = uc($suggested) if ($deprecated_term =~ /^[A-Z]+$/);
> > my $msg_level = \&WARN;
> > $msg_level = \&CHK if ($file);
> > if (&{$msg_level}("DEPRECATED_TERM",
> > - "Use of '$deprecated_term' is deprecated, please '$suggested', instead.\n" . $herecurr) &&
> > + "Use of '$deprecated_term' is controversial - if not required by specification, perhaps '$suggested' instead. See: scripts/deprecated_terms.txt\n" . $herecurr) &&
> > $fix) {
> > $fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($deprecated_term)($|[^A-Za-z@])/$1$suggested$3/;
>
> I think it simpler to avoid emitting this on existing files.
Agreed, it's much simpler. However, my concerns on excluding existing file
checks are:
1. Avoiding existing file checks will still not stop warning patches mentioning
existing deprecated terms.
2. If the term mistakenly comes in newly, it would be hard to check it later.
3. Some future deprecations of terms might be applied to existing uses, as
's/fuck/hug' did.
>
> I do not want to encourage relatively inexperienced people
> to run checkpatch and submit inappropriate patches.
Me, neither. But, I think providing more warnings and references is better for
that. Experienced people would be able to easily ignore the false positives.
Simply limiting checks could allow people submitting inappropriate patches
intorducing new uses of deprecated terms.
Thanks,
SeongJae Park
On Sun, 2020-07-26 at 17:36 +0200, SeongJae Park wrote:
> On Sun, 26 Jul 2020 07:50:54 -0700 Joe Perches <[email protected]> wrote:
[]
> > I do not want to encourage relatively inexperienced people
> > to run checkpatch and submit inappropriate patches.
>
> Me, neither. But, I think providing more warnings and references is better for
> that.
Unfortunately, the inexperienced _do_ in fact run
checkpatch on files and submit inappropriate patches.
It's generally a time sink for the experienced
maintainers to reply.
> Simply limiting checks could allow people submitting inappropriate patches
> intorducing new uses of deprecated terms.
Tradeoffs...
I expect that patches being reviewed by maintainers
are preferred over files being inappropriately changed
by the inexperienced.
Those inappropriate changes should not be encouraged
by tools placed in the hands of the inexperienced.
On Sun, 26 Jul 2020 09:42:06 -0700 Joe Perches <[email protected]> wrote:
> On Sun, 2020-07-26 at 17:36 +0200, SeongJae Park wrote:
> > On Sun, 26 Jul 2020 07:50:54 -0700 Joe Perches <[email protected]> wrote:
> []
> > > I do not want to encourage relatively inexperienced people
> > > to run checkpatch and submit inappropriate patches.
> >
> > Me, neither. But, I think providing more warnings and references is better for
> > that.
>
> Unfortunately, the inexperienced _do_ in fact run
> checkpatch on files and submit inappropriate patches.
>
> It's generally a time sink for the experienced
> maintainers to reply.
>
> > Simply limiting checks could allow people submitting inappropriate patches
> > intorducing new uses of deprecated terms.
>
> Tradeoffs...
>
> I expect that patches being reviewed by maintainers
> are preferred over files being inappropriately changed
> by the inexperienced.
>
> Those inappropriate changes should not be encouraged
> by tools placed in the hands of the inexperienced.
Right, many things are tradeoff. Seems we arrived in the point, though we
still have different opinions. To summarize the pros and cons of my patch from
my perspective:
Pros 1: Handle future terms deprecated with different reasons and coverages.
Pros 2: Inappropriate patches are avoided if the submitters carefully read the
warning messages.
Cons: Careless people could still bother maintainers by not carefully reading
the message and sending inappropriate patches.
To me, the pros still seems larger than the cons. I would like to also again
mention that the maintainer who first reported the problem, Michal, told it's
ok with the explicit messaging. Nonethelss, this is just my opinion.
Attaching the patch addressing your comments for the previous version. The
changes from the previous version are:
- Make the name of reported terms not too verbose
- Avoid unnecessary initialization of the reported terms hash
- Warn multiple deprecated terms in same line
Thanks,
SeongJae Park
p.s I modified my mail formatter to skip adding 'Re:' in the subject. I
thought it's usual behavior of the mailers, but seems it made you only
annoying, sorry. I will not add more 'Re:' for you.
=================================== >8 ========================================
From 169939e24ae98125efcf3af024e6e09cf5cd85f0 Mon Sep 17 00:00:00 2001
From: SeongJae Park <[email protected]>
Date: Sun, 26 Jul 2020 01:14:48 +0200
Subject: [PATCH v3] scripts/deprecatd_terms: provide references
Deprecation of terms could have special rules. For example, 'slave' is
ok for existing usages. Same to 'master', but it's also ok unless it's
used with 'slave'. This commit provides the references for such rules.
Also, because the report became more verbose a little, this commit makes
the report to be made for only one instance of each deprecated term.
Signed-off-by: SeongJae Park <[email protected]>
---
scripts/checkpatch.pl | 6 +++++-
scripts/deprecated_terms.txt | 6 ++++--
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e9fde28eb0de..abed47647fb0 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -721,6 +721,7 @@ sub read_word_corrections {
my %deprecated_terms_fix;
read_word_corrections($deprecated_terms_file, \%deprecated_terms_fix);
my $deprecated_terms = join("|", sort keys %deprecated_terms_fix) if keys %deprecated_terms_fix;
+my %deprecates_reported = map
# Load common spelling mistakes and build regular expression list.
my $misspellings;
@@ -2975,13 +2976,16 @@ sub process {
($in_commit_log || $line =~ /^(?:\+|Subject:)/i)) {
while ($rawline =~ /(?:^|[^a-z@])($deprecated_terms)(?:\b|$|[^a-z@])/gi) {
my $deprecated_term = $1;
+ next if (exists($deprecates_reported{$deprecated_term}));
+ $deprecates_reported{$deprecated_term} = 1;
+
my $suggested = $deprecated_terms_fix{lc($deprecated_term)};
$suggested = ucfirst($suggested) if ($deprecated_term=~ /^[A-Z]/);
$suggested = uc($suggested) if ($deprecated_term =~ /^[A-Z]+$/);
my $msg_level = \&WARN;
$msg_level = \&CHK if ($file);
if (&{$msg_level}("DEPRECATED_TERM",
- "Use of '$deprecated_term' is deprecated, please '$suggested', instead.\n" . $herecurr) &&
+ "Use of '$deprecated_term' is controversial - if not required by specification, perhaps '$suggested' instead. See: scripts/deprecated_terms.txt\n" . $herecurr) &&
$fix) {
$fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($deprecated_term)($|[^A-Za-z@])/$1$suggested$3/;
}
diff --git a/scripts/deprecated_terms.txt b/scripts/deprecated_terms.txt
index 1be27a24187b..d92b9c896fce 100644
--- a/scripts/deprecated_terms.txt
+++ b/scripts/deprecated_terms.txt
@@ -3,8 +3,10 @@
# The format of each line is:
# deprecated||suggested
#
+# If special rules are applied on the terms, please comment those.
+#
+# Refer to "4) Naming" section of Documentation/process/coding-style.rst for
+# below three terms.
blacklist||(denylist|blocklist)
-# For other alternatives of 'slave', Please refer to
-# Documentation/process/coding-style.rst
slave||(secondary|target|...)
whitelist||(allowlist|passlist)
--
2.17.1
On Sun, Jul 26, 2020 at 08:07:48PM +0200, SeongJae Park wrote:
> On Sun, 26 Jul 2020 09:42:06 -0700 Joe Perches <[email protected]> wrote:
>
> > On Sun, 2020-07-26 at 17:36 +0200, SeongJae Park wrote:
> > > On Sun, 26 Jul 2020 07:50:54 -0700 Joe Perches <[email protected]> wrote:
> > []
> > > > I do not want to encourage relatively inexperienced people
> > > > to run checkpatch and submit inappropriate patches.
> > >
> > > Me, neither. But, I think providing more warnings and references is better for
> > > that.
> >
> > Unfortunately, the inexperienced _do_ in fact run
> > checkpatch on files and submit inappropriate patches.
> >
> > It's generally a time sink for the experienced
> > maintainers to reply.
> >
> > > Simply limiting checks could allow people submitting inappropriate patches
> > > intorducing new uses of deprecated terms.
> >
> > Tradeoffs...
> >
> > I expect that patches being reviewed by maintainers
> > are preferred over files being inappropriately changed
> > by the inexperienced.
> >
> > Those inappropriate changes should not be encouraged
> > by tools placed in the hands of the inexperienced.
>
> Right, many things are tradeoff. Seems we arrived in the point, though we
> still have different opinions. To summarize the pros and cons of my patch from
> my perspective:
>
> Pros 1: Handle future terms deprecated with different reasons and coverages.
> Pros 2: Inappropriate patches are avoided if the submitters carefully read the
> warning messages.
> Cons: Careless people could still bother maintainers by not carefully reading
> the message and sending inappropriate patches.
>
> To me, the pros still seems larger than the cons. I would like to also again
> mention that the maintainer who first reported the problem, Michal, told it's
> ok with the explicit messaging. Nonethelss, this is just my opinion.
>
> Attaching the patch addressing your comments for the previous version. The
> changes from the previous version are:
>
> - Make the name of reported terms not too verbose
> - Avoid unnecessary initialization of the reported terms hash
> - Warn multiple deprecated terms in same line
Hi,
Maybe you could split the meaning of --subjective and --strict, and
enable those checks only for --subjective? The test is really hard to do
right: you would have to consider the context and not only mere occurrence
of a word (heh, I even wrote 'blacklisted' here, since it really is about
a night/danger analogy and not political/ethical one).
Best Regards,
Micha??Miros?aw
On Sun, 26 Jul 2020 22:33:28 +0200 "Michał Mirosław" <[email protected]> wrote:
> On Sun, Jul 26, 2020 at 08:07:48PM +0200, SeongJae Park wrote:
> > On Sun, 26 Jul 2020 09:42:06 -0700 Joe Perches <[email protected]> wrote:
> >
> > > On Sun, 2020-07-26 at 17:36 +0200, SeongJae Park wrote:
> > > > On Sun, 26 Jul 2020 07:50:54 -0700 Joe Perches <[email protected]> wrote:
> > > []
> > > > > I do not want to encourage relatively inexperienced people
> > > > > to run checkpatch and submit inappropriate patches.
> > > >
> > > > Me, neither. But, I think providing more warnings and references is better for
> > > > that.
> > >
> > > Unfortunately, the inexperienced _do_ in fact run
> > > checkpatch on files and submit inappropriate patches.
> > >
> > > It's generally a time sink for the experienced
> > > maintainers to reply.
> > >
> > > > Simply limiting checks could allow people submitting inappropriate patches
> > > > intorducing new uses of deprecated terms.
> > >
> > > Tradeoffs...
> > >
> > > I expect that patches being reviewed by maintainers
> > > are preferred over files being inappropriately changed
> > > by the inexperienced.
> > >
> > > Those inappropriate changes should not be encouraged
> > > by tools placed in the hands of the inexperienced.
> >
> > Right, many things are tradeoff. Seems we arrived in the point, though we
> > still have different opinions. To summarize the pros and cons of my patch from
> > my perspective:
> >
> > Pros 1: Handle future terms deprecated with different reasons and coverages.
> > Pros 2: Inappropriate patches are avoided if the submitters carefully read the
> > warning messages.
> > Cons: Careless people could still bother maintainers by not carefully reading
> > the message and sending inappropriate patches.
> >
> > To me, the pros still seems larger than the cons. I would like to also again
> > mention that the maintainer who first reported the problem, Michal, told it's
> > ok with the explicit messaging. Nonethelss, this is just my opinion.
> >
> > Attaching the patch addressing your comments for the previous version. The
> > changes from the previous version are:
> >
> > - Make the name of reported terms not too verbose
> > - Avoid unnecessary initialization of the reported terms hash
> > - Warn multiple deprecated terms in same line
>
> Hi,
>
> Maybe you could split the meaning of --subjective and --strict, and
> enable those checks only for --subjective? The test is really hard to do
> right: you would have to consider the context and not only mere occurrence
> of a word (heh, I even wrote 'blacklisted' here, since it really is about
> a night/danger analogy and not political/ethical one).
I'm concerning if applying the switch and making this patch non-default could
reduce the check coverage. Moreover, IMHO, the deprecation rule of the terms
that described in the 'coding-style.rst' is not so subjective but clear. Also,
the checkpatch's warning/check message for those seems explicit enough to me.
And, the deprecated terms feature is not for this specific terms
(master/slave/blacklist/whitelist) only but general deprecated terms. Maybe we
could add one more rule in the 'deprecated_terms.txt' by adding a comment, say,
"Please add only terms that deprecated with clear rules", for avoiding
introduce of subjective deprecated terms in future, though.
Thanks,
SeongJae Park
>
> Best Regards,
> Michał Mirosław
On Mon, 27 Jul 2020 08:54:41 +0200 SeongJae Park <[email protected]> wrote:
> > > > Unfortunately, the inexperienced _do_ in fact run
> > > > checkpatch on files and submit inappropriate patches.
I don't think I really agree with the "new code only" guideline (where
did this come from, anyway?). 10 years from now any remaining pre-2020
terms will look exceedingly archaic and will get converted at some
point.
Wouldn't be longterm realistic to just bite the bullet now and add these
conversions to the various todo lists?
On Mon, 2020-07-27 at 13:44 -0700, Andrew Morton wrote:
> On Mon, 27 Jul 2020 08:54:41 +0200 SeongJae Park <[email protected]> wrote:
>
> > > > > Unfortunately, the inexperienced _do_ in fact run
> > > > > checkpatch on files and submit inappropriate patches.
>
> I don't think I really agree with the "new code only" guideline (where
> did this come from, anyway?). 10 years from now any remaining pre-2020
> terms will look exceedingly archaic and will get converted at some
> point.
>
> Wouldn't be longterm realistic to just bite the bullet now and add these
> conversions to the various todo lists?
I don't think so.
There's no exclusion list for existing uses
written to external specification.
It's just emitting effectively noisy warnings
on things that should not be changed.
On Mon, 27 Jul 2020 13:49:00 -0700 Joe Perches <[email protected]> wrote:
> On Mon, 2020-07-27 at 13:44 -0700, Andrew Morton wrote:
> > On Mon, 27 Jul 2020 08:54:41 +0200 SeongJae Park <[email protected]> wrote:
> >
> > > > > > Unfortunately, the inexperienced _do_ in fact run
> > > > > > checkpatch on files and submit inappropriate patches.
> >
> > I don't think I really agree with the "new code only" guideline (where
> > did this come from, anyway?). 10 years from now any remaining pre-2020
> > terms will look exceedingly archaic and will get converted at some
> > point.
> >
> > Wouldn't be longterm realistic to just bite the bullet now and add these
> > conversions to the various todo lists?
>
> I don't think so.
>
> There's no exclusion list for existing uses
> written to external specification.
>
> It's just emitting effectively noisy warnings
> on things that should not be changed.
>
Just noticed that this patchset and the followup[1] for sync with inclusive
terms commit[2] are dropped from -mm tree. I admit it could generate some
false positive warnings, though my followup patch[3] makes the message noisy
but gives clear references.
I still believe it's better to provide the messages, but I also know people
could think differently. After all, the biggest part of the initial goal of
this patches is already made by the inclusive terms commit[2]. So, I would
respect the decision.
[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/process/coding-style.rst?id=a5f526ecb075a08c4a082355020166c7fe13ae27
[3] https://lore.kernel.org/lkml/[email protected]/
Thanks,
SeongJae Park