Re: [PATCH v5] checkpatch: extend attributes check to handle more patterns

[Dwaipayan Ray]

On Sun, Oct 25, 2020 at 11:29 PM Joe Perches <joe@xxxxxxxxxxx> wrote:
>
> On Sun, 2020-10-25 at 15:45 +0530, Dwaipayan Ray wrote:
> > It is generally preferred that the macros from
> > include/linux/compiler_attributes.h are used, unless there
> > is a reason not to.
> >
> > checkpatch currently checks __attribute__ for each of
> > packed, aligned, section, printf, scanf, and weak. Other
> > declarations in compiler_attributes.h are not handled.
> >
> > Add a generic test to check the presence of such attributes.
> > Some attributes require more specific handling and are kept
> > separate.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -6155,50 +6155,95 @@ sub process {
> >                       }
> >               }
> >
> >
> > -# Check for __attribute__ packed, prefer __packed
> > +# Check for compiler attributes
> >               if ($realfile !~ m@\binclude/uapi/@ &&
> > -                 $line =~ /\b__attribute__\s*\(\s*\(.*\bpacked\b/) {
> > -                     WARN("PREFER_PACKED",
> > -                          "__packed is preferred over __attribute__((packed))\n" . $herecurr);
> > -             }
> > +                 $rawline =~ /\b__attribute__\s*\(\s*($balanced_parens)\s*\)/) {
>
> Using $rawline would also change comments and that seems wrong.
> Any reason to use $rawline instead of $line?
>
Hi,
Yes I used $line initially but changed it because quoted
strings were being replaced.

Like:
__attribute__((__section__("_ftrace_events")))

$line in this case was:
__attribute__((__section__("XXXXXXXXXXXXXX")))

While $rawline was:
__attribute__((__section__("_ftrace_events")))

So to avoid this problem I changed to $rawline.

Is there any other alternative available perhaps?
Or should I change back to $rawline?

> []
>
> > +                     if (scalar @conv_array > 0 && $conv_possible == 1) {
> > +                             my $replace = join(' ', @conv_array);
> > +                             if (WARN("PREFER_DEFINED_ATTRIBUTE_MACRO",
> > +                                      "$replace is preferred over __attribute__(($attr))\n" . $herecurr) &&
> > +                                     $fix) {
> > +                                     $fixed[$fixlinenr] =~ s/\b__attribute__\s*\(\s*\(\s*\Q$attr\E\s*\)\s*\)/$replace/;
>
> I looks it would be useful to add
>                                         $fixed[$fixlinenr] =~ s/\}\Q$replace\E/} $replace/;
> so there's a space added between } and any replacements.
>
Yes I will do that sure.

Thanks,
Dwaipayan.