Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1471247 > unrolled thread

checkkpatch (in)sanity ?

Back to article view | Back to linux.kernel

Contents

  checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-27 22:50 +0200
    Re: checkkpatch (in)sanity ? "Levin, Alexander" <alexander.levin@verizon.com> - 2016-08-28 03:10 +0200
      Re: checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-28 03:50 +0200
        Re: checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-28 04:30 +0200
        Re: checkkpatch (in)sanity ? "Levin, Alexander" <alexander.levin@verizon.com> - 2016-08-28 04:50 +0200
          Re: checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-28 19:20 +0200
            Re: checkkpatch (in)sanity ? Greg KH <gregkh@linuxfoundation.org> - 2016-08-28 20:00 +0200
            Re: checkkpatch (in)sanity ? "Levin, Alexander" <alexander.levin@verizon.com> - 2016-08-29 00:40 +0200
              Re: checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 01:30 +0200
                Re: checkkpatch (in)sanity ? "Levin, Alexander" <alexander.levin@verizon.com> - 2016-08-29 04:30 +0200
                  Re: checkkpatch (in)sanity ? Christoph Hellwig <hch@infradead.org> - 2016-08-29 10:30 +0200
              Re: [Ksummit-discuss] checkkpatch (in)sanity ? Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-08-29 09:20 +0200
                Re: [Ksummit-discuss] checkkpatch (in)sanity ? Arnd Bergmann <arnd@arndb.de> - 2016-08-29 11:10 +0200
                  Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 14:50 +0200
                    Re: [Ksummit-discuss] checkkpatch (in)sanity ? Josh Triplett <josh@joshtriplett.org> - 2016-08-29 19:20 +0200
                      Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 19:50 +0200
            RE: [Ksummit-discuss] checkkpatch (in)sanity ? "Luck, Tony" <tony.luck@intel.com> - 2016-08-29 19:50 +0200
              Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 20:10 +0200
                Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 20:50 +0200
                Re: [Ksummit-discuss] checkkpatch (in)sanity ? Josh Triplett <josh@joshtriplett.org> - 2016-08-29 21:10 +0200
                Re: [Ksummit-discuss] checkkpatch (in)sanity ? Arnd Bergmann <arnd@arndb.de> - 2016-08-29 23:10 +0200
        Re: checkkpatch (in)sanity ? Kalle Valo <kvalo@codeaurora.org> - 2016-08-29 13:20 +0200
          Re: checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 14:40 +0200
            Re: checkkpatch (in)sanity ? Kalle Valo <kvalo@codeaurora.org> - 2016-08-29 20:10 +0200
              Re: checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 21:10 +0200
              Re: [Ksummit-discuss] checkkpatch (in)sanity ? Arnd Bergmann <arnd@arndb.de> - 2016-08-29 23:10 +0200
      Re: [Ksummit-discuss] checkkpatch (in)sanity ? Alexey Dobriyan <adobriyan@gmail.com> - 2016-08-28 10:00 +0200
        Re: [Ksummit-discuss] checkkpatch (in)sanity ? Julia Lawall <julia.lawall@lip6.fr> - 2016-08-28 12:00 +0200
          Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-28 22:00 +0200
            Re: [Ksummit-discuss] checkkpatch (in)sanity ? Jiri Kosina <jikos@kernel.org> - 2016-08-28 22:40 +0200
            Re: [Ksummit-discuss] checkkpatch (in)sanity ? Dennis Kaarsemaker <dennis@kaarsemaker.net> - 2016-08-28 23:30 +0200
              Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 00:00 +0200
    Re: [Ksummit-discuss] checkkpatch (in)sanity ? Dan Carpenter <dan.carpenter@oracle.com> - 2016-08-29 21:10 +0200
      Re: [Ksummit-discuss] checkkpatch (in)sanity ? Dan Carpenter <dan.carpenter@oracle.com> - 2016-08-29 21:20 +0200
        Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 21:40 +0200
      Re: [Ksummit-discuss] checkkpatch (in)sanity ? Josh Triplett <josh@joshtriplett.org> - 2016-08-29 21:20 +0200
        Re: [Ksummit-discuss] checkkpatch (in)sanity ? Joe Perches <joe@perches.com> - 2016-08-29 21:30 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1472121 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Monday 29 August 2016, Joe Perches wrote:
> On Mon, 2016-08-29 at 17:46 +0000, Luck, Tony wrote:
> > > 
> > > 80 columns is simply silly when dealing with either
> > > long identifiers or many levels of indentation.
> > > 
> > > One thing that 80 column limit does do is encourage
> > > shorter identifiers and fewer levels of indentation.
> > > 
> > > Generally, both of those are good things.
> > I think the main complaint with the limit is that people fix it by simply
> > breaking the long line, which often makes for less readable code.
> > 
> > Perhaps there would be less pushback on this if checkpatch also
> > complained about clumsily broken long lines and offered the advice
> > to restructure the code with helper functions etc. to avoid deep
> > indentation?
> 
> It suggests that already for 6+ leading tabs, but some more
> intelligence for nominally ugly added line breaks would
> definitely help.
> 
> Using longish simple identifiers or multiple dereferences
> can make the line breaks at 80 columns silly.

My preferred personal guideline for the maximum indentation is
the area that a function takes up in the editor. It's sometimes
ok to have really long functions (hundreds of lines), but only
with one or two levels of indentation. It's also sometimes ok
to have five or six levels of intendation, but only if the
function is really short and you can see immediately how
it works.

Having a long function with multiple nested loops and conditions
is almost always a problem for readability, and we should be
able to detect this programatically if we want to.

There are more accurate ways to tell if you are getting
too complex (e.g. CONFIG_GCC_PLUGIN_CYC_COMPLEXITY), but that
becomes harder to warn about.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1471733

Joe Perches <joe@perches.com> writes:

> On Sat, 2016-08-27 at 21:06 -0400, Levin, Alexander wrote:
>> On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote:
>> > On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
>> > > On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
>> > > > 
>> > > >     - Making checkpatch check for (some) of the stable kernel rules
>> > > >     (and possibly recommend adding the stable@ tag in certain cases?).
>> > > >       - Depends on: making checkpatch sane again
>> > > > >This sounds interesting.  What do you mean by "sane"?
>> > Sasha, can you expand your thoughts here please?
>> Sure. I have 2.5 concerns about the state of checkpatch:
> []
>> > Most all of the trivial spacing stuff can easily be
>> > ignored either by a human determining what's important
>> > or by using command line options like --ignore=spacing
>> 1.
>> This is the wrong default. By default checkpatch shouldn't be showing trivial
>> issues that encourage folks to try and work around them and as a result
>> produce worse code.
>> 
>> Look at the 80 character limit warning for example, what good does it do?
>
> That argument's been done several times. It keeps Linus happy.
> I don't care one way or another.
>
> I think the biggest issue is the seriousness that some people
> take checkpatch messages as dicta instead of ignorable bleats.

I wish that checkpatch would have a way to enable/disable warnings per
directory (or file). For example, there would be
drivers/net/wireless/ath/ath10k/.checkpatch which would disable the
warnings are not suitable for ath10k for one reason or another:

'MSLEEP',
'USLEEP_RANGE',
'PRINTK_WITHOUT_KERN_LEVEL',
'NETWORKING_BLOCK_COMMENT_STYLE',
'BLOCK_COMMENT_STYLE',
'LINUX_VERSION_CODE',
'COMPLEX_MACRO',
'PREFER_DEV_LEVEL',
'PREFER_PR_LEVEL',
'COMPARISON_TO_NULL',
'BIT_MACRO',
'CONSTANT_COMPARISON',
'MACRO_WITH_FLOW_CONTROL'

Currently my workaround is to have a custom ath10k-check script[1] which
runs checkpatch with those checks disabled. Oh, and it also filters out
some of the warnings based on the symbol it is located in.

https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath10k/ath10k-check

-- 
Kalle Valo

[toc] | [prev] | [next] | [standalone]

#1471796

On Mon, 2016-08-29 at 14:15 +0300, Kalle Valo wrote:
> I wish that checkpatch would have a way to enable/disable warnings per
> directory (or file). For example, there would be
> drivers/net/wireless/ath/ath10k/.checkpatch which would disable the
> warnings are not suitable for ath10k for one reason or another:
> 
> 'MSLEEP',
> 'USLEEP_RANGE',
> 'PRINTK_WITHOUT_KERN_LEVEL',
> 'NETWORKING_BLOCK_COMMENT_STYLE',
> 'BLOCK_COMMENT_STYLE',
> 'LINUX_VERSION_CODE',
> 'COMPLEX_MACRO',
> 'PREFER_DEV_LEVEL',
> 'PREFER_PR_LEVEL',
> 'COMPARISON_TO_NULL',
> 'BIT_MACRO',
> 'CONSTANT_COMPARISON',
> 'MACRO_WITH_FLOW_CONTROL'
> 
> Currently my workaround is to have a custom ath10k-check script[1] which
> runs checkpatch with those checks disabled. Oh, and it also filters out
> some of the warnings based on the symbol it is located in.
> 
> https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath10k/ath10k-check

Hey Kalle:

I looked at your script (which also does compilation
and sparse checking) I don't see how a .checkpatch_conf
hierarchy helps you much there as you've added all those
long symbol name long line avoidance bits.

Also, there'd be a lot of rework to the globals in
checkpatch for per-directory specific overrides if someone
fed it files in multiple directories like

checkpatch.pl <patchfile touching lib/kernel/include>

A couple btw's:

Why avoid the printk, sleep or macro tests?

And this for ath10k_core_register_work:

    ('ath10k_core_register_work', 'RETURN_VOID'),

and the code associated to it:

err:
	/* TODO: It's probably a good idea to release device from the driver
	 * but calling device_release_driver() here will cause a deadlock.
	 */
	return;
}

ia avoided a few times in the kernel by using a bare ";"
instead of "return;" before the function closing brace.

It's maybe unfortunate that gcc / c spec doesn't allow
jumping to a label just before the function close brace.

[toc] | [prev] | [next] | [standalone]

#1472034

Joe Perches <joe@perches.com> writes:

> On Mon, 2016-08-29 at 14:15 +0300, Kalle Valo wrote:
>> I wish that checkpatch would have a way to enable/disable warnings per
>> directory (or file). For example, there would be
>> drivers/net/wireless/ath/ath10k/.checkpatch which would disable the
>> warnings are not suitable for ath10k for one reason or another:
>> 
>> 'MSLEEP',
>> 'USLEEP_RANGE',
>> 'PRINTK_WITHOUT_KERN_LEVEL',
>> 'NETWORKING_BLOCK_COMMENT_STYLE',
>> 'BLOCK_COMMENT_STYLE',
>> 'LINUX_VERSION_CODE',
>> 'COMPLEX_MACRO',
>> 'PREFER_DEV_LEVEL',
>> 'PREFER_PR_LEVEL',
>> 'COMPARISON_TO_NULL',
>> 'BIT_MACRO',
>> 'CONSTANT_COMPARISON',
>> 'MACRO_WITH_FLOW_CONTROL'
>> 
>> Currently my workaround is to have a custom ath10k-check script[1] which
>> runs checkpatch with those checks disabled. Oh, and it also filters out
>> some of the warnings based on the symbol it is located in.
>> 
>> https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath10k/ath10k-check
>
> Hey Kalle:
>
> I looked at your script (which also does compilation
> and sparse checking) I don't see how a .checkpatch_conf
> hierarchy helps you much there as you've added all those
> long symbol name long line avoidance bits.

Yeah, it would not completely replace my script. But there's now quite a
difference with checkpatch parameters what other people use and what I
use.

> Also, there'd be a lot of rework to the globals in
> checkpatch for per-directory specific overrides if someone
> fed it files in multiple directories like
>
> checkpatch.pl <patchfile touching lib/kernel/include>

Oh, that's a good point. ath10k patches only touch one directory so I
didn't think of this.

> A couple btw's:
>
> Why avoid the printk, sleep or macro tests?

Actually I don't remember anymore :) I should have documented that in
the script.

(Runs some tests)

PRINTK_WITHOUT_KERN_LEVEL: I think I can enable this again, IIRC earlier
it gave useless warnings in ath10k_warn() & co.

MSLEEP: I think this is just noise, we don't care if it's actually 20 ms
even if ask for 10 ms. That's the minimum time to wait, not the maximum
(from our/HW point of view).

drivers/net/wireless/ath/ath10k/ahb.c:422: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/ahb.c:427: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/ahb.c:432: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/ahb.c:437: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/ahb.c:442: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/pci.c:2244: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/pci.c:2251: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
drivers/net/wireless/ath/ath10k/pci.c:2275: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt

USLEEP_RANGE: I don't remember why I disabled this, most likely just
because of the msleep noise above

drivers/net/wireless/ath/ath10k/pci.c:2672: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt

COMPLEX_MACRO: Can't remember this one either, I guess I just didn't
find a good solution to shut up checkpatch. But that was a long time
ago, it might be fixable now.

drivers/net/wireless/ath/ath10k/wmi.h:315: Macros with complex values should be enclosed in parentheses
drivers/net/wireless/ath/ath10k/wmi.h:6344: Macros with complex values should be enclosed in parentheses

BIT_MACRO: there's a lot of warnings from this one, the benefit from
changing all of them is questionable so I just disabled it.

drivers/net/wireless/ath/ath10k/rx_desc.h:676: Prefer using the BIT macro

> And this for ath10k_core_register_work:
>
>     ('ath10k_core_register_work', 'RETURN_VOID'),
>
> and the code associated to it:
>
> err:
> 	/* TODO: It's probably a good idea to release device from the driver
> 	 * but calling device_release_driver() here will cause a deadlock.
> 	 */
> 	return;
> }
>
> ia avoided a few times in the kernel by using a bare ";"
> instead of "return;" before the function closing brace.

Heh, didn't know that.

> It's maybe unfortunate that gcc / c spec doesn't allow
> jumping to a label just before the function close brace.

Indeed.

I find checkpatch very useful to maintain certain coding style in ath10k
and I don't need to worry small details like whitespace. I just need to
disable some of the warnings so that they don't hide the real warnings
I'm interested about.

-- 
Kalle Valo

[toc] | [prev] | [next] | [standalone]

#1472059

On Mon, 2016-08-29 at 21:01 +0300, Kalle Valo wrote:

> there's now quite a
> difference with checkpatch parameters what other people use and what I
> use.
[]
> I find checkpatch very useful to maintain certain coding style in ath10k
> and I don't need to worry small details like whitespace. I just need to
> disable some of the warnings so that they don't hide the real warnings
> I'm interested about.

I don't see a conflict here.

The entire point of classifying all of those checkpatch
message types was to allow exactly what you are doing.

[toc] | [prev] | [next] | [standalone]

#1472120 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Monday 29 August 2016, Kalle Valo wrote:
> MSLEEP: I think this is just noise, we don't care if it's actually 20 ms
> even if ask for 10 ms. That's the minimum time to wait, not the maximum
> (from our/HW point of view).
> 
> drivers/net/wireless/ath/ath10k/ahb.c:422: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> drivers/net/wireless/ath/ath10k/ahb.c:427: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> drivers/net/wireless/ath/ath10k/ahb.c:432: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> drivers/net/wireless/ath/ath10k/ahb.c:437: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> drivers/net/wireless/ath/ath10k/ahb.c:442: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> drivers/net/wireless/ath/ath10k/pci.c:2244: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> drivers/net/wireless/ath/ath10k/pci.c:2251: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> drivers/net/wireless/ath/ath10k/pci.c:2275: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
> 

I've seen way too many patches addressing this warning in various ways that
do not improve readability or behavior of the driver. Typically a driver
calls "msleep(1)" in a loop as a way to poll for an event that will happen
at some point in the future but that generates no IRQ or other event we
can wait for, the stuff that used to be handled with "yield" a long time
ago.

Now every third caller of usleep_range() uses '1000' as the minimum time
and an arbitrary upper bound.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1471301 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Sat, Aug 27, 2016 at 09:06:13PM -0400, Levin, Alexander via Ksummit-discuss wrote:
> 3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
> perl code that a fair amount of people don't know how to deal with. In 4.8 it's
> 6142 lines, making it the 124th largest source file in the kernel, well within
> the top 1% of source files in the kernel.
> 
> This combination of size/language pushes people away from being involved in
> what is supposed to be a central tool and gives them a reason to never use
> it again after they see results they don't agree with (rather than fixing it).

It is a textbook example of what's wrong with Perl. Instead of parsing
C code like compilers do, the script is one big pile of regexes.
It mostly works ("doing its job" in perlspeak) because people mostly
follow the coding style.

Regarding individual warnings: some are good (RETURN_VOID, DATE_TIME,
USE_NEGATIVE_ERRNO), some are OK given kernel style of allocating memory
but the rationale is bogus (UNNECESSARY_CASTS, linking to userspace
example of malloc() returning "int"!).

And then there is ALLOC_SIZEOF_STRUCT which advocates "kmalloc(sizeof(*p))".

The problem is that c-h.pl generates noise in the commit history and
makes git-blame less useful than it can be.

I for one given up on it more or less since its introduction.

[toc] | [prev] | [next] | [standalone]

#1471317 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Sun, 28 Aug 2016, Alexey Dobriyan wrote:

> On Sat, Aug 27, 2016 at 09:06:13PM -0400, Levin, Alexander via Ksummit-discuss wrote:
> > 3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
> > perl code that a fair amount of people don't know how to deal with. In 4.8 it's
> > 6142 lines, making it the 124th largest source file in the kernel, well within
> > the top 1% of source files in the kernel.
> >
> > This combination of size/language pushes people away from being involved in
> > what is supposed to be a central tool and gives them a reason to never use
> > it again after they see results they don't agree with (rather than fixing it).
>
> It is a textbook example of what's wrong with Perl. Instead of parsing
> C code like compilers do, the script is one big pile of regexes.
> It mostly works ("doing its job" in perlspeak) because people mostly
> follow the coding style.

Parsing is slow.  Perfect parsing is impossible due to configuration
options.  There is definitely a place for regexps in checking code.
Perhaps there is a better way to express the regexps, or to provide
regexps for integration into the checkpatch infrastructure.

> Regarding individual warnings: some are good (RETURN_VOID, DATE_TIME,
> USE_NEGATIVE_ERRNO), some are OK given kernel style of allocating memory
> but the rationale is bogus (UNNECESSARY_CASTS, linking to userspace
> example of malloc() returning "int"!).
>
> And then there is ALLOC_SIZEOF_STRUCT which advocates "kmalloc(sizeof(*p))".
>
> The problem is that c-h.pl generates noise in the commit history and
> makes git-blame less useful than it can be.

Could it be that this is a problem with git blame, rather than with
checkpatch?  Last year there was a discussion on this list about how there
is an option to git blame that will cause it to step through the history,
and not show only the most recent patch that has modified a given line.

julia

>
> I for one given up on it more or less since its introduction.
> _______________________________________________
> Ksummit-discuss mailing list
> Ksummit-discuss@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
>

[toc] | [prev] | [next] | [standalone]

#1471446 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Sun, 2016-08-28 at 11:59 +0200, Julia Lawall wrote:
> On Sun, 28 Aug 2016, Alexey Dobriyan wrote:
[]
> > The problem is that c-h.pl generates noise in the commit history and
> > makes git-blame less useful than it can be.
> 
> Could it be that this is a problem with git blame, rather than with
> checkpatch?  Last year there was a discussion on this list about how there
> is an option to git blame that will cause it to step through the history,
> and not show only the most recent patch that has modified a given line.

It is more or less an ease-of-use limitation of git blame.

There are some that want an ncurses only version of git blame
that could
use arrow-key style navigation for historical commit
line-ranges.

git gui blame kind of works, but it's not ncurses/text based.
git-cola kind of works too, but it's not text based either.

Are there other existing tools for blame history viewing?

[toc] | [prev] | [next] | [standalone]

#1471451 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Sun, 28 Aug 2016, Joe Perches wrote:

> Are there other existing tools for blame history viewing?

fugitive vim plugin has 'Gblame' command, which I personally find rather 
useful, and given the text-oriented nature could potentially be useful for 
your needs.

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1471472 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On zo, 2016-08-28 at 12:52 -0700, Joe Perches wrote:
> On Sun, 2016-08-28 at 11:59 +0200, Julia Lawall wrote:
> > 
> > On Sun, 28 Aug 2016, Alexey Dobriyan wrote:
> []
> > 
> > > 
> > > The problem is that c-h.pl generates noise in the commit history
> > > and
> > > makes git-blame less useful than it can be.
> > Could it be that this is a problem with git blame, rather than with
> > checkpatch?  Last year there was a discussion on this list about
> > how there
> > is an option to git blame that will cause it to step through the
> > history,
> > and not show only the most recent patch that has modified a given
> > line.
> It is more or less an ease-of-use limitation of git blame.
> 
> There are some that want an ncurses only version of git blame
> that could
> use arrow-key style navigation for historical commit
> line-ranges.
> 
> git gui blame kind of works, but it's not ncurses/text based.
> git-cola kind of works too, but it's not text based either.
> 
> Are there other existing tools for blame history viewing?

tig has a neat way of doing blame history digging: do a `tig blame
filename`, select a line and hit the comma key to re-blame from the
parent of the commit that was blamed for that line.

(hat tip to Jeff King who pointed out this trick recently)

D.

[toc] | [prev] | [next] | [standalone]

#1471481 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Sun, 2016-08-28 at 23:24 +0200, Dennis Kaarsemaker wrote:

> > There are some that want an ncurses only version of git blame
> > that could use arrow-key style navigation for historical commit
> > line-ranges.
> > 
> > git gui blame kind of works, but it's not ncurses/text based.
> > git-cola kind of works too, but it's not text based either.
> > 
> > Are there other existing tools for blame history viewing?
> tig has a neat way of doing blame history digging: do a `tig blame
> filename`, select a line and hit the comma key to re-blame from the
> parent of the commit that was blamed for that line.

Thanks, of course I neglected to mention tig.

What I think some here want is the ability to view
back and forth from old to new rather than just split
the window horizontally with the commit content.

Basically, apply the patch hunks for a specific range
to see color coded changes rather just show the entire
patch in a separate view.

Sure, seeing the commit log and patch is useful.

Seeing the block of code pre and post patch for the
specific section can be more useful.

[toc] | [prev] | [next] | [standalone]

#1472065 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

I would like a couple changes which you know already:

1) Get rid of PREFER_ETHER_ADDR_COPY and similar because the people who
send checkpatch.pl fixes aren't qualified to say when it's legal or not
so they sometimes introduce bugs.

2) We could put some text in the output of --file output to say that if
it's not a staging patch, then we don't care about just making
checkpatch happy.  So consider if it is a waste of maintainer time
before sending.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1472067 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Mon, Aug 29, 2016 at 12:10:20PM -0700, Josh Triplett wrote:
> On Mon, Aug 29, 2016 at 10:06:18PM +0300, Dan Carpenter wrote:
> > I would like a couple changes which you know already:
> > 
> > 1) Get rid of PREFER_ETHER_ADDR_COPY and similar because the people who
> > send checkpatch.pl fixes aren't qualified to say when it's legal or not
> > so they sometimes introduce bugs.
> 
> I do think we should have *something* that catches such things.
> Perhaps not checkpatch.pl, though.  Perhaps a compiler plugin that
> generates additional warnings, and can perhaps use more global
> information to determine legality?

Perhaps.  But that shouldn't delay us from deleting this code which just
encourages newbies to introduce bugs.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1472081 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Mon, 2016-08-29 at 22:17 +0300, Dan Carpenter wrote:
> On Mon, Aug 29, 2016 at 12:10:20PM -0700, Josh Triplett wrote:
> > On Mon, Aug 29, 2016 at 10:06:18PM +0300, Dan Carpenter wrote:
> > > I would like a couple changes which you know already:
> > > 
> > > 1) Get rid of PREFER_ETHER_ADDR_COPY and similar because the people who
> > > send checkpatch.pl fixes aren't qualified to say when it's legal or not
> > > so they sometimes introduce bugs.
> > I do think we should have *something* that catches such things.
> > Perhaps not checkpatch.pl, though.  Perhaps a compiler plugin that
> > generates additional warnings, and can perhaps use more global
> > information to determine legality?
> Perhaps.  But that shouldn't delay us from deleting this code which just
> encourages newbies to introduce bugs.

You could send a patch.

I still kinda like the --force option

https://patchwork.kernel.org/patch/5814071/

[toc] | [prev] | [next] | [standalone]

#1472072 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Mon, Aug 29, 2016 at 10:06:18PM +0300, Dan Carpenter wrote:
> I would like a couple changes which you know already:
> 
> 1) Get rid of PREFER_ETHER_ADDR_COPY and similar because the people who
> send checkpatch.pl fixes aren't qualified to say when it's legal or not
> so they sometimes introduce bugs.

I do think we should have *something* that catches such things.
Perhaps not checkpatch.pl, though.  Perhaps a compiler plugin that
generates additional warnings, and can perhaps use more global
information to determine legality?

[toc] | [prev] | [next] | [standalone]

#1472076 — Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Mon, 2016-08-29 at 12:10 -0700, Josh Triplett wrote:
> On Mon, Aug 29, 2016 at 10:06:18PM +0300, Dan Carpenter wrote:
> > 
> > I would like a couple changes which you know already:
> > 
> > 1) Get rid of PREFER_ETHER_ADDR_COPY and similar because the people who
> > send checkpatch.pl fixes aren't qualified to say when it's legal or not
> > so they sometimes introduce bugs.
> I do think we should have *something* that catches such things.
> Perhaps not checkpatch.pl, though.  Perhaps a compiler plugin that
> generates additional warnings, and can perhaps use more global
> information to determine legality?

nit: validity rather than legality.

There are still rather a lot of these.

$ git grep -E "\bmem.*,\s*(ETH_ALEN|6)\s*\);" | wc -l
1776

Dunno if any of them are in performance sensitive
areas where it actually matters.

Someone, I forget who, had a concern about the
object being set possibly being in a struct where
it's possible for the alignment of the set object
to be altered by another change like adding a new
member.

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web