| Message ID | 1452427000-4520-2-git-send-email-mst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 2016-01-10 at 13:56 +0200, Michael S. Tsirkin wrote: > SMP-only barriers were missing in checkpatch.pl > > Refactor code slightly to make adding more variants easier. [] > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > @@ -5116,7 +5116,25 @@ sub process { > } > } > # check for memory barriers without a comment. > - if ($line =~ /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) { > + > + my $barriers = qr{ > + mb| > + rmb| > + wmb| > + read_barrier_depends > + }x; > + my $smp_barriers = qr{ > + store_release| > + load_acquire| > + store_mb| > + ($barriers) > + }x; If I use a variable called $smp_barriers, I'd expect it to actually be the smp_barriers, not to have to prefix it with smp_ before using it. my $smp_barriers = qr{ smp_store_release| smp_load_acquire| smp_store_mb| smp_read_barrier_depends }x; or my $smp_barriers = qr{ smp_(?:store_release|load_acquire|store_mb|read_barrier_depends) }x; > + my $all_barriers = qr{ > + $barriers| > + smp_($smp_barriers) > + }x; And this shouldn't have a capture group. my $all_barriers = qr{ $barriers| $smp_barriers }x; > + > + if ($line =~ /\b($all_barriers)\s*\(/) { This doesn't need the capture group either (?:all_barriers) > if (!ctx_has_comment($first_line, $linenr)) > { > WARN("MEMORY_BARRIER", > "memory barrier without > comment\n" . $herecurr);
On Sun, 2016-01-10 at 07:07 -0800, Joe Perches wrote: > On Sun, 2016-01-10 at 13:56 +0200, Michael S. Tsirkin wrote: > > SMP-only barriers were missing in checkpatch.pl > > > > Refactor code slightly to make adding more variants easier. > [] > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > If I use a variable called $smp_barriers, I'd expect > it to actually be the smp_barriers, not to have to > prefix it with smp_ before using it. > > my $smp_barriers = qr{ > smp_store_release| > smp_load_acquire| > smp_store_mb| > smp_read_barrier_depends That's missing (?:barriers) too. btw: shouldn't this also have smp_mb__(?:before|after)_atomic ?
On Sun, Jan 10, 2016 at 07:07:05AM -0800, Joe Perches wrote: > On Sun, 2016-01-10 at 13:56 +0200, Michael S. Tsirkin wrote: > > SMP-only barriers were missing in checkpatch.pl > > > > Refactor code slightly to make adding more variants easier. > [] > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] > > @@ -5116,7 +5116,25 @@ sub process { > > } > > } > > # check for memory barriers without a comment. > > - if ($line =~ /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) { > > + > > + my $barriers = qr{ > > + mb| > > + rmb| > > + wmb| > > + read_barrier_depends > > + }x; > > + my $smp_barriers = qr{ > > + store_release| > > + load_acquire| > > + store_mb| > > + ($barriers) > > + }x; > > If I use a variable called $smp_barriers, I'd expect > it to actually be the smp_barriers, not to have to > prefix it with smp_ before using it. > > my $smp_barriers = qr{ > smp_store_release| > smp_load_acquire| > smp_store_mb| > smp_read_barrier_depends > }x; > > or > > my $smp_barriers = qr{ > smp_(?:store_release|load_acquire|store_mb|read_barrier_depends) > }x; > Yes but virt barriers (added in patch 3) are same things but prefixed with virt_. So we need the stems without smp_ prefix. If smp_barriers is too confusing we'll just need to give them some other name. How about: my $smp_barrier_stems ? > > + my $all_barriers = qr{ > > + $barriers| > > + smp_($smp_barriers) > > + }x; > > And this shouldn't have a capture group. > > my $all_barriers = qr{ > $barriers| > $smp_barriers > }x; > > + > > + if ($line =~ /\b($all_barriers)\s*\(/) { > > This doesn't need the capture group either (?:all_barriers) > > > if (!ctx_has_comment($first_line, $linenr)) > > { > > WARN("MEMORY_BARRIER", > > "memory barrier without > > comment\n" . $herecurr);
On Sun, Jan 10, 2016 at 07:17:31AM -0800, Joe Perches wrote: > On Sun, 2016-01-10 at 07:07 -0800, Joe Perches wrote: > > On Sun, 2016-01-10 at 13:56 +0200, Michael S. Tsirkin wrote: > > > SMP-only barriers were missing in checkpatch.pl > > > > > > Refactor code slightly to make adding more variants easier. > > [] > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] > > If I use a variable called $smp_barriers, I'd expect > > it to actually be the smp_barriers, not to have to > > prefix it with smp_ before using it. > > > > my $smp_barriers = qr{ > > smp_store_release| > > smp_load_acquire| > > smp_store_mb| > > smp_read_barrier_depends > > That's missing (?:barriers) too. My version has it but need to add ?: to avoid a capture group. > btw: shouldn't this also have > smp_mb__(?:before|after)_atomic > ? Good catch, included in the next version.
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 2b3c228..97b8b62 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5116,7 +5116,25 @@ sub process { } } # check for memory barriers without a comment. - if ($line =~ /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) { + + my $barriers = qr{ + mb| + rmb| + wmb| + read_barrier_depends + }x; + my $smp_barriers = qr{ + store_release| + load_acquire| + store_mb| + ($barriers) + }x; + my $all_barriers = qr{ + $barriers| + smp_($smp_barriers) + }x; + + if ($line =~ /\b($all_barriers)\s*\(/) { if (!ctx_has_comment($first_line, $linenr)) { WARN("MEMORY_BARRIER", "memory barrier without comment\n" . $herecurr);
SMP-only barriers were missing in checkpatch.pl Refactor code slightly to make adding more variants easier. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- scripts/checkpatch.pl | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)