* Checkpatch error on trace events macros
@ 2013-07-29 19:52 Sarah Sharp
2013-07-29 20:02 ` Joe Perches
2013-07-30 1:30 ` Li Zefan
0 siblings, 2 replies; 12+ messages in thread
From: Sarah Sharp @ 2013-07-29 19:52 UTC (permalink / raw)
To: Andy Whitcroft, Joe Perches; +Cc: linux-kernel
Hi Andy and Joe,
Checkpatch is complaining when code adds new trace events macros:
sarah@xanatos:~/git/kernels/xhci$ git am -s ~/Maildir.fetchmail/.to-apply
Applying: xhci: add traces for debug messages in xhci_address_device()
ERROR: Macros with complex values should be enclosed in parenthesis
#86: FILE: drivers/usb/host/xhci-trace.h:15:
+#define TRACE_SYSTEM xhci-hcd
ERROR: Macros with complex values should be enclosed in parenthesis
#115: FILE: drivers/usb/host/xhci-trace.h:44:
+#define TRACE_INCLUDE_PATH .
ERROR: Macros with complex values should be enclosed in parenthesis
#118: FILE: drivers/usb/host/xhci-trace.h:47:
+#define TRACE_INCLUDE_FILE xhci-trace
total: 3 errors, 0 warnings, 169 lines checked
The macros have to be defined that way for trace events to work.
Can you fix checkpatch not to complain about trace event macros?
Thanks,
Sarah Sharp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Checkpatch error on trace events macros
2013-07-29 19:52 Checkpatch error on trace events macros Sarah Sharp
@ 2013-07-29 20:02 ` Joe Perches
2013-07-29 21:23 ` Sarah Sharp
2013-07-30 1:30 ` Li Zefan
1 sibling, 1 reply; 12+ messages in thread
From: Joe Perches @ 2013-07-29 20:02 UTC (permalink / raw)
To: Sarah Sharp; +Cc: Andy Whitcroft, linux-kernel
On Mon, 2013-07-29 at 12:52 -0700, Sarah Sharp wrote:
> Hi Andy and Joe,
Hi Sarah.
> Checkpatch is complaining when code adds new trace events macros:
>
> sarah@xanatos:~/git/kernels/xhci$ git am -s ~/Maildir.fetchmail/.to-apply
> Applying: xhci: add traces for debug messages in xhci_address_device()
> ERROR: Macros with complex values should be enclosed in parenthesis
> #86: FILE: drivers/usb/host/xhci-trace.h:15:
> +#define TRACE_SYSTEM xhci-hcd
<shrug>
I think these are suboptimal as the files should use
underscores rather than dashes.
checkpatch sees this as a subtraction which really
should have parentheses.
> ERROR: Macros with complex values should be enclosed in parenthesis
> #115: FILE: drivers/usb/host/xhci-trace.h:44:
> +#define TRACE_INCLUDE_PATH .
>
> ERROR: Macros with complex values should be enclosed in parenthesis
> #118: FILE: drivers/usb/host/xhci-trace.h:47:
> +#define TRACE_INCLUDE_FILE xhci-trace
Don't expect to checkpatch to be perfect.
It's not.
It's a stupid little tool good for some things
and good for highlighting areas that might need
another look.
Use your judgment about everything it spews.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Checkpatch error on trace events macros
2013-07-29 20:02 ` Joe Perches
@ 2013-07-29 21:23 ` Sarah Sharp
2013-07-29 21:48 ` Joe Perches
0 siblings, 1 reply; 12+ messages in thread
From: Sarah Sharp @ 2013-07-29 21:23 UTC (permalink / raw)
To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel
On Mon, Jul 29, 2013 at 01:02:44PM -0700, Joe Perches wrote:
> On Mon, 2013-07-29 at 12:52 -0700, Sarah Sharp wrote:
> > Hi Andy and Joe,
>
> Hi Sarah.
>
> > Checkpatch is complaining when code adds new trace events macros:
> >
> > sarah@xanatos:~/git/kernels/xhci$ git am -s ~/Maildir.fetchmail/.to-apply
> > Applying: xhci: add traces for debug messages in xhci_address_device()
> > ERROR: Macros with complex values should be enclosed in parenthesis
> > #86: FILE: drivers/usb/host/xhci-trace.h:15:
> > +#define TRACE_SYSTEM xhci-hcd
>
> <shrug>
>
> I think these are suboptimal as the files should use
> underscores rather than dashes.
The norm in the USB subsystem is to use dashes in filenames. I think
it's suboptimal to have to use the shift key at all when typing
filenames. We have different preferences here, and different reasons
for those preferences, but there is no "should" here, just opinions.
> checkpatch sees this as a subtraction which really
> should have parentheses.
I see.
> > ERROR: Macros with complex values should be enclosed in parenthesis
> > #115: FILE: drivers/usb/host/xhci-trace.h:44:
> > +#define TRACE_INCLUDE_PATH .
> >
> > ERROR: Macros with complex values should be enclosed in parenthesis
> > #118: FILE: drivers/usb/host/xhci-trace.h:47:
> > +#define TRACE_INCLUDE_FILE xhci-trace
>
> Don't expect to checkpatch to be perfect.
>
> It's not.
>
> It's a stupid little tool good for some things
> and good for highlighting areas that might need
> another look.
>
> Use your judgment about everything it spews.
If checkpatch spews warnings and errors, that makes it basically
useless as a git pre-commit hook.
Sigh, I suppose I'll just add a '|| true' to the end of the line and
deal with it.
Sarah Sharp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Checkpatch error on trace events macros
2013-07-29 21:23 ` Sarah Sharp
@ 2013-07-29 21:48 ` Joe Perches
0 siblings, 0 replies; 12+ messages in thread
From: Joe Perches @ 2013-07-29 21:48 UTC (permalink / raw)
To: Sarah Sharp; +Cc: Andy Whitcroft, LKML
On Mon, 2013-07-29 at 14:23 -0700, Sarah Sharp wrote:
> On Mon, Jul 29, 2013 at 01:02:44PM -0700, Joe Perches wrote:
> > On Mon, 2013-07-29 at 12:52 -0700, Sarah Sharp wrote:
[]
> > > sarah@xanatos:~/git/kernels/xhci$ git am -s ~/Maildir.fetchmail/.to-apply
> > > Applying: xhci: add traces for debug messages in xhci_address_device()
> > > ERROR: Macros with complex values should be enclosed in parenthesis
> > > #86: FILE: drivers/usb/host/xhci-trace.h:15:
> > > +#define TRACE_SYSTEM xhci-hcd
> >
> > <shrug>
> >
> > I think these are suboptimal as the files should use
> > underscores rather than dashes.
It's ~3:2 in favor of _ in the tree.
$ git ls-files | awk -F"/" '{print $NF;}' | grep "-" | wc -l
8078
$ git ls-files | awk -F"/" '{print $NF;}' | grep "_" | wc -l
12577
closer to 1:1 if you include all the directory names too.
$ git ls-files | grep "-" | wc -l
11821
$ git ls-files | grep "_" | wc -l
13005
> The norm in the USB subsystem is to use dashes in filenames.
Not really.
$ git ls-files drivers/usb | awk -F"/" '{print $NF;}' | grep "-" | wc -l
168
$ git ls-files drivers/usb | awk -F"/" '{print $NF;}' | grep "_" | wc -l
177
> there is no "should" here, just opinions.
Given the "I think", we agree!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Checkpatch error on trace events macros
2013-07-29 19:52 Checkpatch error on trace events macros Sarah Sharp
2013-07-29 20:02 ` Joe Perches
@ 2013-07-30 1:30 ` Li Zefan
2013-07-30 1:58 ` Joe Perches
1 sibling, 1 reply; 12+ messages in thread
From: Li Zefan @ 2013-07-30 1:30 UTC (permalink / raw)
To: Sarah Sharp; +Cc: Andy Whitcroft, Joe Perches, linux-kernel
On 2013/7/30 3:52, Sarah Sharp wrote:
> Hi Andy and Joe,
>
> Checkpatch is complaining when code adds new trace events macros:
>
> sarah@xanatos:~/git/kernels/xhci$ git am -s ~/Maildir.fetchmail/.to-apply
> Applying: xhci: add traces for debug messages in xhci_address_device()
> ERROR: Macros with complex values should be enclosed in parenthesis
> #86: FILE: drivers/usb/host/xhci-trace.h:15:
> +#define TRACE_SYSTEM xhci-hcd
>
> ERROR: Macros with complex values should be enclosed in parenthesis
> #115: FILE: drivers/usb/host/xhci-trace.h:44:
> +#define TRACE_INCLUDE_PATH .
>
> ERROR: Macros with complex values should be enclosed in parenthesis
> #118: FILE: drivers/usb/host/xhci-trace.h:47:
> +#define TRACE_INCLUDE_FILE xhci-trace
>
> total: 3 errors, 0 warnings, 169 lines checked
>
>
> The macros have to be defined that way for trace events to work.
yeah, that's true, and we always just ignore chechpatch complaints
when it comes to TRACE_EVENT macros.
> Can you fix checkpatch not to complain about trace event macros?
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Checkpatch error on trace events macros
2013-07-30 1:30 ` Li Zefan
@ 2013-07-30 1:58 ` Joe Perches
2013-07-30 2:06 ` Li Zefan
0 siblings, 1 reply; 12+ messages in thread
From: Joe Perches @ 2013-07-30 1:58 UTC (permalink / raw)
To: Li Zefan; +Cc: Sarah Sharp, Andy Whitcroft, linux-kernel
On Tue, 2013-07-30 at 09:30 +0800, Li Zefan wrote:
> On 2013/7/30 3:52, Sarah Sharp wrote:
> > Hi Andy and Joe,
> >
> > Checkpatch is complaining when code adds new trace events macros:
> >
> > sarah@xanatos:~/git/kernels/xhci$ git am -s ~/Maildir.fetchmail/.to-apply
> > Applying: xhci: add traces for debug messages in xhci_address_device()
> > ERROR: Macros with complex values should be enclosed in parenthesis
> > #86: FILE: drivers/usb/host/xhci-trace.h:15:
> > +#define TRACE_SYSTEM xhci-hcd
> >
> > ERROR: Macros with complex values should be enclosed in parenthesis
> > #115: FILE: drivers/usb/host/xhci-trace.h:44:
> > +#define TRACE_INCLUDE_PATH .
> >
> > ERROR: Macros with complex values should be enclosed in parenthesis
> > #118: FILE: drivers/usb/host/xhci-trace.h:47:
> > +#define TRACE_INCLUDE_FILE xhci-trace
> >
> > total: 3 errors, 0 warnings, 169 lines checked
> >
> >
> > The macros have to be defined that way for trace events to work.
>
> yeah, that's true, and we always just ignore chechpatch complaints
> when it comes to TRACE_EVENT macros.
>
> > Can you fix checkpatch not to complain about trace event macros?
> >
So what are these TRACE_<FOO> defines that need
excluding from the "complex values" check?
Anything other than
TRACE_SYSTEM
TRACE_INCLUDE_FILE
TRACE_INCLUDE_PATH
?
samples/trace_events/trace-events-sample.h
only has those 3.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Checkpatch error on trace events macros
2013-07-30 1:58 ` Joe Perches
@ 2013-07-30 2:06 ` Li Zefan
2013-07-30 2:36 ` Joe Perches
0 siblings, 1 reply; 12+ messages in thread
From: Li Zefan @ 2013-07-30 2:06 UTC (permalink / raw)
To: Joe Perches; +Cc: Sarah Sharp, Andy Whitcroft, linux-kernel
On 2013/7/30 9:58, Joe Perches wrote:
> On Tue, 2013-07-30 at 09:30 +0800, Li Zefan wrote:
>> On 2013/7/30 3:52, Sarah Sharp wrote:
>>> Hi Andy and Joe,
>>>
>>> Checkpatch is complaining when code adds new trace events macros:
>>>
>>> sarah@xanatos:~/git/kernels/xhci$ git am -s ~/Maildir.fetchmail/.to-apply
>>> Applying: xhci: add traces for debug messages in xhci_address_device()
>>> ERROR: Macros with complex values should be enclosed in parenthesis
>>> #86: FILE: drivers/usb/host/xhci-trace.h:15:
>>> +#define TRACE_SYSTEM xhci-hcd
>>>
>>> ERROR: Macros with complex values should be enclosed in parenthesis
>>> #115: FILE: drivers/usb/host/xhci-trace.h:44:
>>> +#define TRACE_INCLUDE_PATH .
>>>
>>> ERROR: Macros with complex values should be enclosed in parenthesis
>>> #118: FILE: drivers/usb/host/xhci-trace.h:47:
>>> +#define TRACE_INCLUDE_FILE xhci-trace
>>>
>>> total: 3 errors, 0 warnings, 169 lines checked
>>>
>>>
>>> The macros have to be defined that way for trace events to work.
>>
>> yeah, that's true, and we always just ignore chechpatch complaints
>> when it comes to TRACE_EVENT macros.
>>
>>> Can you fix checkpatch not to complain about trace event macros?
>>>
>
> So what are these TRACE_<FOO> defines that need
> excluding from the "complex values" check?
>
> Anything other than
>
> TRACE_SYSTEM
> TRACE_INCLUDE_FILE
> TRACE_INCLUDE_PATH
>
> ?
>
> samples/trace_events/trace-events-sample.h
> only has those 3.
>
Try:
# scripts/checkpatch.pl --file include/trace/events/*
You'll see numerous errors. :)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Checkpatch error on trace events macros
2013-07-30 2:06 ` Li Zefan
@ 2013-07-30 2:36 ` Joe Perches
2013-07-30 3:04 ` Li Zefan
0 siblings, 1 reply; 12+ messages in thread
From: Joe Perches @ 2013-07-30 2:36 UTC (permalink / raw)
To: Li Zefan; +Cc: Sarah Sharp, Andy Whitcroft, linux-kernel
On Tue, 2013-07-30 at 10:06 +0800, Li Zefan wrote:
> On 2013/7/30 9:58, Joe Perches wrote:
> > So what are these TRACE_<FOO> defines that need
> > excluding from the "complex values" check?
> >
> > Anything other than
> >
> > TRACE_SYSTEM
> > TRACE_INCLUDE_FILE
> > TRACE_INCLUDE_PATH
> >
> > ?
> >
> > samples/trace_events/trace-events-sample.h
> > only has those 3.
> >
>
> Try:
> # scripts/checkpatch.pl --file include/trace/events/*
>
> You'll see numerous errors. :)
Nope, you'll see numerous whitespace defects, but no
actual errors.
If you run with:
--ignore=spacing,long_line,code_indent,leading_space,printf_l,split_string,space_before_tab,trailing_whitespace,line_continuations
it's flawless.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Checkpatch error on trace events macros
2013-07-30 2:36 ` Joe Perches
@ 2013-07-30 3:04 ` Li Zefan
2013-07-30 3:10 ` Joe Perches
0 siblings, 1 reply; 12+ messages in thread
From: Li Zefan @ 2013-07-30 3:04 UTC (permalink / raw)
To: Joe Perches; +Cc: Sarah Sharp, Andy Whitcroft, linux-kernel
On 2013/7/30 10:36, Joe Perches wrote:
> On Tue, 2013-07-30 at 10:06 +0800, Li Zefan wrote:
>> On 2013/7/30 9:58, Joe Perches wrote:
>>> So what are these TRACE_<FOO> defines that need
>>> excluding from the "complex values" check?
>>>
>>> Anything other than
>>>
>>> TRACE_SYSTEM
>>> TRACE_INCLUDE_FILE
>>> TRACE_INCLUDE_PATH
>>>
>>> ?
>>>
>>> samples/trace_events/trace-events-sample.h
>>> only has those 3.
>>>
>>
>> Try:
>> # scripts/checkpatch.pl --file include/trace/events/*
>>
>> You'll see numerous errors. :)
>
> Nope, you'll see numerous whitespace defects, but no
> actual errors.
>
> If you run with:
>
> --ignore=spacing,long_line,code_indent,leading_space,printf_l,split_string,space_before_tab,trailing_whitespace,line_continuations
>
Serious? I'd just not run checkpatch.pl. ;)
> it's flawless.
>
The "complex values" check complaints come from many places in include/trace/events/*,
and I'm not going to check where and why.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Checkpatch error on trace events macros
2013-07-30 3:04 ` Li Zefan
@ 2013-07-30 3:10 ` Joe Perches
2013-07-30 3:25 ` Li Zefan
0 siblings, 1 reply; 12+ messages in thread
From: Joe Perches @ 2013-07-30 3:10 UTC (permalink / raw)
To: Li Zefan; +Cc: Sarah Sharp, Andy Whitcroft, linux-kernel
On Tue, 2013-07-30 at 11:04 +0800, Li Zefan wrote:
> On 2013/7/30 10:36, Joe Perches wrote:
> > On Tue, 2013-07-30 at 10:06 +0800, Li Zefan wrote:
> >> On 2013/7/30 9:58, Joe Perches wrote:
> >>> So what are these TRACE_<FOO> defines that need
> >>> excluding from the "complex values" check?
> >>>
> >>> Anything other than
> >>>
> >>> TRACE_SYSTEM
> >>> TRACE_INCLUDE_FILE
> >>> TRACE_INCLUDE_PATH
> >>>
> >>> ?
> >>>
> >>> samples/trace_events/trace-events-sample.h
> >>> only has those 3.
> >>>
> >>
> >> Try:
> >> # scripts/checkpatch.pl --file include/trace/events/*
> >>
> >> You'll see numerous errors. :)
> >
> > Nope, you'll see numerous whitespace defects, but no
> > actual errors.
> >
> > If you run with:
> >
> > --ignore=spacing,long_line,code_indent,leading_space,printf_l,split_string,space_before_tab,trailing_whitespace,line_continuations
> >
>
> Serious? I'd just not run checkpatch.pl. ;)
Your choice. It's all whitespace and %Lx stuff.
I think the reports are actual style defects.
The line continuations and split strings uses are
pretty poor there too.
> The "complex values" check complaints come from many places in include/trace/events/*,
> and I'm not going to check where and why.
Nor I.
You haven't answered my question either.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Checkpatch error on trace events macros
2013-07-30 3:10 ` Joe Perches
@ 2013-07-30 3:25 ` Li Zefan
2013-07-30 18:17 ` [PATCH] checkpatch: Ignore #define TRACE_<foo> macros Joe Perches
0 siblings, 1 reply; 12+ messages in thread
From: Li Zefan @ 2013-07-30 3:25 UTC (permalink / raw)
To: Joe Perches; +Cc: Sarah Sharp, Andy Whitcroft, linux-kernel
On 2013/7/30 11:10, Joe Perches wrote:
> On Tue, 2013-07-30 at 11:04 +0800, Li Zefan wrote:
>> On 2013/7/30 10:36, Joe Perches wrote:
>>> On Tue, 2013-07-30 at 10:06 +0800, Li Zefan wrote:
>>>> On 2013/7/30 9:58, Joe Perches wrote:
>>>>> So what are these TRACE_<FOO> defines that need
>>>>> excluding from the "complex values" check?
>>>>>
>>>>> Anything other than
>>>>>
>>>>> TRACE_SYSTEM
>>>>> TRACE_INCLUDE_FILE
>>>>> TRACE_INCLUDE_PATH
>>>>>
>>>>> ?
>>>>>
>>>>> samples/trace_events/trace-events-sample.h
>>>>> only has those 3.
>>>>>
>>>>
>>>> Try:
>>>> # scripts/checkpatch.pl --file include/trace/events/*
>>>>
>>>> You'll see numerous errors. :)
>>>
>>> Nope, you'll see numerous whitespace defects, but no
>>> actual errors.
>>>
>>> If you run with:
>>>
>>> --ignore=spacing,long_line,code_indent,leading_space,printf_l,split_string,space_before_tab,trailing_whitespace,line_continuations
>>>
>>
>> Serious? I'd just not run checkpatch.pl. ;)
>
> Your choice. It's all whitespace and %Lx stuff.
> I think the reports are actual style defects.
> The line continuations and split strings uses are
> pretty poor there too.
>
>> The "complex values" check complaints come from many places in include/trace/events/*,
>> and I'm not going to check where and why.
>
> Nor I.
> You haven't answered my question either.
>
Oh, I overlooked it.
TRACE_SYSTEM defines the directory name in /sys/kernel/debug/tracing/events. A trace
event belongs to a trace system:
# cat /sys/kernel/debug/tracing/available_events
ext3:ext3_free_inode
ext3:ext3_request_inode
...
ext3 is the SYSTEM name.
TRACE_INCLUDE_FILE is needed if the .h filename is different than the SYSTEM name.
If the .h file is not in include/trace/events, then you must use TRACE_INCLUDE_PATH
to specify where the file is.
I'm not sure if there're any other maros need special treatment.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] checkpatch: Ignore #define TRACE_<foo> macros
2013-07-30 3:25 ` Li Zefan
@ 2013-07-30 18:17 ` Joe Perches
0 siblings, 0 replies; 12+ messages in thread
From: Joe Perches @ 2013-07-30 18:17 UTC (permalink / raw)
To: Andrew Morton; +Cc: Sarah Sharp, Li Zefan, Andy Whitcroft, linux-kernel
The tracing subsystem uses slightly odd #defines
to set path/directory locations for include files.
These #defines can cause false positives for the
complex macro tests so add exclusions for these
specific #defines (TRACE_SYSTEM, TRACE_INCLUDE_FILE,
TRACE_INCLUDE_PATH).
Signed-off-by: Joe Perches <joe@perches.com>
---
scripts/checkpatch.pl | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2ee9eb7..998ad8f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3374,7 +3374,8 @@ sub process {
$dstat !~ /^for\s*$Constant$/ && # for (...)
$dstat !~ /^for\s*$Constant\s+(?:$Ident|-?$Constant)$/ && # for (...) bar()
$dstat !~ /^do\s*{/ && # do {...
- $dstat !~ /^\({/) # ({...
+ $dstat !~ /^\({/ && # ({...
+ $ctx !~ /^.\s*#\s*define\s+TRACE_(?:SYSTEM|INCLUDE_FILE|INCLUDE_PATH)\b/)
{
$ctx =~ s/\n*$//;
my $herectx = $here . "\n";
^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-07-30 18:17 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-29 19:52 Checkpatch error on trace events macros Sarah Sharp
2013-07-29 20:02 ` Joe Perches
2013-07-29 21:23 ` Sarah Sharp
2013-07-29 21:48 ` Joe Perches
2013-07-30 1:30 ` Li Zefan
2013-07-30 1:58 ` Joe Perches
2013-07-30 2:06 ` Li Zefan
2013-07-30 2:36 ` Joe Perches
2013-07-30 3:04 ` Li Zefan
2013-07-30 3:10 ` Joe Perches
2013-07-30 3:25 ` Li Zefan
2013-07-30 18:17 ` [PATCH] checkpatch: Ignore #define TRACE_<foo> macros Joe Perches
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.