archive mirror
 help / color / mirror / Atom feed
From: David Gow <>
To: Joe Perches <>
Cc: Dan Carpenter <>,
	shuah <>,
	Brendan Higgins <>,
	Andrew Morton <>,
	Kees Cook <>,
Subject: Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list
Date: Thu, 31 Oct 2019 00:12:22 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

I tend to agree that it's better to either fix or ignore checkpatch
than to arbitrarily change things in cases like this where checkpatch
is obviously wrong. Equally, it certainly seems that there isn't an
obvious way of modifying checkpatch that will both not cause other
problems and not add another arbitrary name check. The main concern
about just leaving the checkpatch errors in is that people might be
automatically rejecting changes (or worse, the whole kselftest/test
pull request) if checkpatch errors are present. I'm not sure how
likely that is, but I can understand the desire to be careful, since
relatively minor changes have delayed KUnit changes before.

So, there are a few options, I guess:
- Hack around the issue in the patch (as this v7 is doing). Ugly, but
does at least mean that this change won't trigger any automated
rejection-of-checkpatch-errors people might be doing. (Even if, I
think we agree, automatically rejecting anything with checkpatch
warnings is not really correct.)
- Accept that tests (and other functions) with "for_each" in the name
like this are rare enough that it's not worth the complexity of
supporting it in checkpatch, and taking v6 as-is with the checkpatch
- Modify checkpatch to handle this in some other way (e.g., only if
the name doesn't include "test"): I don't think there's a perfectly
clean way of doing this.
- Modify checkpatch to make this ERROR a WARNING instead, since we
know this check has some flaws in this test, and potentially future
- Re-send v6 with a note about the checkpatch warning in the
description, so that it's easier to tell if one or more of these

Is there some combination of the above that sounds good?

-- David

  reply	other threads:[~2019-10-31  7:12 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-24 22:46 David Gow
2019-10-29 13:00 ` shuah
2019-10-30  8:02   ` David Gow
2019-10-30 10:42     ` Dan Carpenter
2019-10-30 16:27       ` shuah
2019-10-30 16:35         ` Brendan Higgins
2019-10-30 17:18           ` Joe Perches
2019-10-31  8:51             ` Brendan Higgins
2019-10-31 10:07               ` Joe Perches
2019-10-31 10:20               ` Dan Carpenter
2019-10-30 18:46         ` Dan Carpenter
2019-10-30 19:15           ` Joe Perches
2019-10-31  6:59             ` Dan Carpenter
2019-11-01 10:50             ` Rasmus Villemoes
2019-10-30 19:12         ` Dan Carpenter
2019-10-30 19:23           ` Joe Perches
2019-10-31  7:12             ` David Gow [this message]
2019-10-31  7:42               ` Dan Carpenter
2019-11-01 16:49             ` shuah
2019-10-30 16:31       ` Joe Perches
2019-10-31 18:50 ` Kees Cook
2019-11-01 10:25   ` David Gow

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='' \ \ \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the '\''list'\'' doubly linked list' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).