Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   SeongJae Park <sj38.park@gmail.com>
To:     Joe Perches <joe@perches.com>
Cc:     SeongJae Park <sj38.park@gmail.com>,
        =?UTF-8?q?Micha=C5=82=20Miros=C5=82aw?= <mirq-linux@rere.qmqm.pl>,
        SeongJae Park <sjpark@amazon.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, apw@canonical.com,
        colin.king@canonical.com, jslaby@suse.cz, pavel@ucw.cz,
        SeongJae Park <sjpark@amazon.de>
Subject: Re: Re: Re: Re: Re: checkpatch: support deprecated terms checking
Date:   Sun, 26 Jul 2020 17:36:01 +0200
Message-Id: <20200726153601.13855-1-sj38.park@gmail.com>
In-Reply-To: <dae471a789ee6e0bda5e641eb8e376fd03667415.camel@perches.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, 26 Jul 2020 07:50:54 -0700 Joe Perches <joe@perches.com> 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