* [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement @ 2017-01-30 16:31 Maksymilian Piechota 2017-01-30 16:44 ` Greg Kroah-Hartman 0 siblings, 1 reply; 8+ messages in thread From: Maksymilian Piechota @ 2017-01-30 16:31 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: maksymilianpiechota, linux-kernel, devel This patch fixes the checkpatch.pl warning: WARNING: Statements should start on a tabstop Signed-off-by: Maksymilian Piechota <maksymilianpiechota@gmail.com> --- drivers/staging/wlan-ng/prism2mgmt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/wlan-ng/prism2mgmt.c b/drivers/staging/wlan-ng/prism2mgmt.c index 16fb2d3..2d67125 100644 --- a/drivers/staging/wlan-ng/prism2mgmt.c +++ b/drivers/staging/wlan-ng/prism2mgmt.c @@ -1308,7 +1308,7 @@ int prism2mgmt_wlansniff(struct wlandevice *wlandev, void *msgp) hw->sniffhdr = 0; wlandev->netdev->type = ARPHRD_IEEE80211_PRISM; } else - if ((msg->wlanheader.status == + if ((msg->wlanheader.status == P80211ENUM_msgitem_status_data_ok) && (msg->wlanheader.data == P80211ENUM_truth_true)) { hw->sniffhdr = 1; -- 2.1.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement 2017-01-30 16:31 [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement Maksymilian Piechota @ 2017-01-30 16:44 ` Greg Kroah-Hartman 2017-01-31 4:00 ` Joe Perches 0 siblings, 1 reply; 8+ messages in thread From: Greg Kroah-Hartman @ 2017-01-30 16:44 UTC (permalink / raw) To: Maksymilian Piechota; +Cc: linux-kernel, devel On Mon, Jan 30, 2017 at 11:31:42AM -0500, Maksymilian Piechota wrote: > This patch fixes the checkpatch.pl warning: > > WARNING: Statements should start on a tabstop > > Signed-off-by: Maksymilian Piechota <maksymilianpiechota@gmail.com> > --- > drivers/staging/wlan-ng/prism2mgmt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/wlan-ng/prism2mgmt.c b/drivers/staging/wlan-ng/prism2mgmt.c > index 16fb2d3..2d67125 100644 > --- a/drivers/staging/wlan-ng/prism2mgmt.c > +++ b/drivers/staging/wlan-ng/prism2mgmt.c > @@ -1308,7 +1308,7 @@ int prism2mgmt_wlansniff(struct wlandevice *wlandev, void *msgp) > hw->sniffhdr = 0; > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM; > } else > - if ((msg->wlanheader.status == > + if ((msg->wlanheader.status == > P80211ENUM_msgitem_status_data_ok) > && (msg->wlanheader.data == P80211ENUM_truth_true)) { > hw->sniffhdr = 1; Hm, this all doesn't look correct now, does it? Please fix up the whole if statement here. thanks, greg k-h ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement 2017-01-30 16:44 ` Greg Kroah-Hartman @ 2017-01-31 4:00 ` Joe Perches 2017-01-31 11:04 ` Maksymilian Piechota 0 siblings, 1 reply; 8+ messages in thread From: Joe Perches @ 2017-01-31 4:00 UTC (permalink / raw) To: Greg Kroah-Hartman, Maksymilian Piechota; +Cc: linux-kernel, devel On Mon, 2017-01-30 at 17:44 +0100, Greg Kroah-Hartman wrote: > On Mon, Jan 30, 2017 at 11:31:42AM -0500, Maksymilian Piechota wrote: > > This patch fixes the checkpatch.pl warning: > > > > WARNING: Statements should start on a tabstop > > > > Signed-off-by: Maksymilian Piechota <maksymilianpiechota@gmail.com> > > --- > > drivers/staging/wlan-ng/prism2mgmt.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/wlan-ng/prism2mgmt.c b/drivers/staging/wlan-ng/prism2mgmt.c > > index 16fb2d3..2d67125 100644 > > --- a/drivers/staging/wlan-ng/prism2mgmt.c > > +++ b/drivers/staging/wlan-ng/prism2mgmt.c > > @@ -1308,7 +1308,7 @@ int prism2mgmt_wlansniff(struct wlandevice *wlandev, void *msgp) > > hw->sniffhdr = 0; > > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM; > > } else > > - if ((msg->wlanheader.status == > > + if ((msg->wlanheader.status == > > P80211ENUM_msgitem_status_data_ok) > > && (msg->wlanheader.data == P80211ENUM_truth_true)) { > > hw->sniffhdr = 1; > > Hm, this all doesn't look correct now, does it? Please fix up the whole > if statement here. Ideally, it'd look something like: /* Set the driver state */ /* Do we want the prism2 header? */ if (msg->prismheader.status == P80211ENUM_msgitem_status_data_ok && msg->prismheader.data == P80211ENUM_truth_true) { hw->sniffhdr = 0; wlandev->netdev->type = ARPHRD_IEEE80211_PRISM; } else if (msg->wlanheader.status == P80211ENUM_msgitem_status_data_ok && msg->wlanheader.data == P80211ENUM_truth_true) { hw->sniffhdr = 1; wlandev->netdev->type = ARPHRD_IEEE80211_PRISM; } else { wlandev->netdev->type = ARPHRD_IEEE80211; } with the unnecessary parentheses removed, the logical continuations at the end-of-line, and the else if on a single line. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement 2017-01-31 4:00 ` Joe Perches @ 2017-01-31 11:04 ` Maksymilian Piechota 2017-01-31 11:18 ` Joe Perches 0 siblings, 1 reply; 8+ messages in thread From: Maksymilian Piechota @ 2017-01-31 11:04 UTC (permalink / raw) To: Joe Perches; +Cc: Greg Kroah-Hartman, Maksymilian Piechota, linux-kernel, devel On Mon, Jan 30, 2017 at 08:00:36PM -0800, Joe Perches wrote: > On Mon, 2017-01-30 at 17:44 +0100, Greg Kroah-Hartman wrote: > > On Mon, Jan 30, 2017 at 11:31:42AM -0500, Maksymilian Piechota wrote: > > > This patch fixes the checkpatch.pl warning: > > > > > > WARNING: Statements should start on a tabstop > > > > > > Signed-off-by: Maksymilian Piechota <maksymilianpiechota@gmail.com> > > > --- > > > drivers/staging/wlan-ng/prism2mgmt.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/wlan-ng/prism2mgmt.c b/drivers/staging/wlan-ng/prism2mgmt.c > > > index 16fb2d3..2d67125 100644 > > > --- a/drivers/staging/wlan-ng/prism2mgmt.c > > > +++ b/drivers/staging/wlan-ng/prism2mgmt.c > > > @@ -1308,7 +1308,7 @@ int prism2mgmt_wlansniff(struct wlandevice *wlandev, void *msgp) > > > hw->sniffhdr = 0; > > > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM; > > > } else > > > - if ((msg->wlanheader.status == > > > + if ((msg->wlanheader.status == > > > P80211ENUM_msgitem_status_data_ok) > > > && (msg->wlanheader.data == P80211ENUM_truth_true)) { > > > hw->sniffhdr = 1; > > > > Hm, this all doesn't look correct now, does it? Please fix up the whole > > if statement here. > > Ideally, it'd look something like: > > /* Set the driver state */ > /* Do we want the prism2 header? */ > if (msg->prismheader.status == P80211ENUM_msgitem_status_data_ok && > msg->prismheader.data == P80211ENUM_truth_true) { > hw->sniffhdr = 0; > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM; > } else if (msg->wlanheader.status == P80211ENUM_msgitem_status_data_ok && > msg->wlanheader.data == P80211ENUM_truth_true) { > hw->sniffhdr = 1; > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM; > } else { > wlandev->netdev->type = ARPHRD_IEEE80211; > } > > with the unnecessary parentheses removed, > the logical continuations at the end-of-line, > and the else if on a single line. > I must admit it looks better, but this way we get 2 warnings instead of 1 (before my changes). What is the policy? Can we ignore more warnings in order to get cleaner code? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement 2017-01-31 11:04 ` Maksymilian Piechota @ 2017-01-31 11:18 ` Joe Perches 2017-01-31 11:33 ` Maksymilian Piechota 0 siblings, 1 reply; 8+ messages in thread From: Joe Perches @ 2017-01-31 11:18 UTC (permalink / raw) To: Maksymilian Piechota; +Cc: Greg Kroah-Hartman, linux-kernel, devel On Tue, 2017-01-31 at 06:04 -0500, Maksymilian Piechota wrote: > On Mon, Jan 30, 2017 at 08:00:36PM -0800, Joe Perches wrote: > > On Mon, 2017-01-30 at 17:44 +0100, Greg Kroah-Hartman wrote: > > > On Mon, Jan 30, 2017 at 11:31:42AM -0500, Maksymilian Piechota wrote: > > > > This patch fixes the checkpatch.pl warning: > > > > > > > > WARNING: Statements should start on a tabstop > > > > > > > > Signed-off-by: Maksymilian Piechota <maksymilianpiechota@gmail.com> > > > > --- > > > > drivers/staging/wlan-ng/prism2mgmt.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/staging/wlan-ng/prism2mgmt.c b/drivers/staging/wlan-ng/prism2mgmt.c > > > > index 16fb2d3..2d67125 100644 > > > > --- a/drivers/staging/wlan-ng/prism2mgmt.c > > > > +++ b/drivers/staging/wlan-ng/prism2mgmt.c > > > > @@ -1308,7 +1308,7 @@ int prism2mgmt_wlansniff(struct wlandevice *wlandev, void *msgp) > > > > hw->sniffhdr = 0; > > > > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM; > > > > } else > > > > - if ((msg->wlanheader.status == > > > > + if ((msg->wlanheader.status == > > > > P80211ENUM_msgitem_status_data_ok) > > > > && (msg->wlanheader.data == P80211ENUM_truth_true)) { > > > > hw->sniffhdr = 1; > > > > > > Hm, this all doesn't look correct now, does it? Please fix up the whole > > > if statement here. > > > > Ideally, it'd look something like: > > > > /* Set the driver state */ > > /* Do we want the prism2 header? */ > > if (msg->prismheader.status == P80211ENUM_msgitem_status_data_ok && > > msg->prismheader.data == P80211ENUM_truth_true) { > > hw->sniffhdr = 0; > > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM; > > } else if (msg->wlanheader.status == P80211ENUM_msgitem_status_data_ok && > > msg->wlanheader.data == P80211ENUM_truth_true) { > > hw->sniffhdr = 1; > > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM; > > } else { > > wlandev->netdev->type = ARPHRD_IEEE80211; > > } > > > > with the unnecessary parentheses removed, > > the logical continuations at the end-of-line, > > and the else if on a single line. > > > > I must admit it looks better, but this way we get 2 warnings instead of > 1 (before my changes). What is the policy? Can we ignore more warnings > in order to get cleaner code? Yes please. checkpatch is just a guide, it's brainless. The reason these lines are > 80 columns is overly long/verbose identifiers. If you really want to clean up the code here, the P90211ENUM_ prefixes are a bit misleading as they all are #define and not enums at all. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement 2017-01-31 11:18 ` Joe Perches @ 2017-01-31 11:33 ` Maksymilian Piechota 2017-01-31 12:07 ` Joe Perches 0 siblings, 1 reply; 8+ messages in thread From: Maksymilian Piechota @ 2017-01-31 11:33 UTC (permalink / raw) To: Joe Perches; +Cc: Maksymilian Piechota, Greg Kroah-Hartman, linux-kernel, devel On Tue, Jan 31, 2017 at 03:18:45AM -0800, Joe Perches wrote: > On Tue, 2017-01-31 at 06:04 -0500, Maksymilian Piechota wrote: > > On Mon, Jan 30, 2017 at 08:00:36PM -0800, Joe Perches wrote: > > > On Mon, 2017-01-30 at 17:44 +0100, Greg Kroah-Hartman wrote: > > > > On Mon, Jan 30, 2017 at 11:31:42AM -0500, Maksymilian Piechota wrote: > > > > > This patch fixes the checkpatch.pl warning: > > > > > > > > > > WARNING: Statements should start on a tabstop > > > > > > > > > > Signed-off-by: Maksymilian Piechota <maksymilianpiechota@gmail.com> > > > > > --- > > > > > drivers/staging/wlan-ng/prism2mgmt.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/staging/wlan-ng/prism2mgmt.c b/drivers/staging/wlan-ng/prism2mgmt.c > > > > > index 16fb2d3..2d67125 100644 > > > > > --- a/drivers/staging/wlan-ng/prism2mgmt.c > > > > > +++ b/drivers/staging/wlan-ng/prism2mgmt.c > > > > > @@ -1308,7 +1308,7 @@ int prism2mgmt_wlansniff(struct wlandevice *wlandev, void *msgp) > > > > > hw->sniffhdr = 0; > > > > > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM; > > > > > } else > > > > > - if ((msg->wlanheader.status == > > > > > + if ((msg->wlanheader.status == > > > > > P80211ENUM_msgitem_status_data_ok) > > > > > && (msg->wlanheader.data == P80211ENUM_truth_true)) { > > > > > hw->sniffhdr = 1; > > > > > > > > Hm, this all doesn't look correct now, does it? Please fix up the whole > > > > if statement here. > > > > > > Ideally, it'd look something like: > > > > > > /* Set the driver state */ > > > /* Do we want the prism2 header? */ > > > if (msg->prismheader.status == P80211ENUM_msgitem_status_data_ok && > > > msg->prismheader.data == P80211ENUM_truth_true) { > > > hw->sniffhdr = 0; > > > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM; > > > } else if (msg->wlanheader.status == P80211ENUM_msgitem_status_data_ok && > > > msg->wlanheader.data == P80211ENUM_truth_true) { > > > hw->sniffhdr = 1; > > > wlandev->netdev->type = ARPHRD_IEEE80211_PRISM; > > > } else { > > > wlandev->netdev->type = ARPHRD_IEEE80211; > > > } > > > > > > with the unnecessary parentheses removed, > > > the logical continuations at the end-of-line, > > > and the else if on a single line. > > > > > > > I must admit it looks better, but this way we get 2 warnings instead of > > 1 (before my changes). What is the policy? Can we ignore more warnings > > in order to get cleaner code? > > Yes please. > > checkpatch is just a guide, it's brainless. > > The reason these lines are > 80 columns is > overly long/verbose identifiers. > > If you really want to clean up the code here, > the P90211ENUM_ prefixes are a bit misleading > as they all are #define and not enums at all. > But would you like me to remove this prefixes for all of the enums from p80211types.h? Are you sure it won't cause any symbol conflicts? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement 2017-01-31 11:33 ` Maksymilian Piechota @ 2017-01-31 12:07 ` Joe Perches 2017-01-31 12:20 ` Maksymilian Piechota 0 siblings, 1 reply; 8+ messages in thread From: Joe Perches @ 2017-01-31 12:07 UTC (permalink / raw) To: Maksymilian Piechota; +Cc: Greg Kroah-Hartman, linux-kernel, devel On Tue, 2017-01-31 at 06:33 -0500, Maksymilian Piechota wrote: > On Tue, Jan 31, 2017 at 03:18:45AM -0800, Joe Perches wrote: > > checkpatch is just a guide, it's brainless. > > > > The reason these lines are > 80 columns is > > overly long/verbose identifiers. > > > > If you really want to clean up the code here, > > the P90211ENUM_ prefixes are a bit misleading > > as they all are #define and not enums at all. > > > But would you like me to remove this prefixes for all of the enums from > p80211types.h? Are you sure it won't cause any symbol conflicts? No, I don't want that, I'd prefer you think about it. Also, a useful effort would be to (from the README) TODO: [] - move to use the in-kernel wireless stack where most all of the P80211 code would be removed. Anyway, sure, use checkpatch as a tool to help when learning the process of how to submit patches properly. Then move on to more thoroughly understand a block of code in the kernel that can be improved with cleaner style and logic and bug fixes you could submit. That's be much more appreciated than random 80 column fixups and strict checkpatch compliance done without a thorough understanding of the code. In any case, welcome, hope you stick around. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement 2017-01-31 12:07 ` Joe Perches @ 2017-01-31 12:20 ` Maksymilian Piechota 0 siblings, 0 replies; 8+ messages in thread From: Maksymilian Piechota @ 2017-01-31 12:20 UTC (permalink / raw) To: Joe Perches; +Cc: Maksymilian Piechota, Greg Kroah-Hartman, linux-kernel, devel On Tue, Jan 31, 2017 at 04:07:04AM -0800, Joe Perches wrote: > On Tue, 2017-01-31 at 06:33 -0500, Maksymilian Piechota wrote: > > On Tue, Jan 31, 2017 at 03:18:45AM -0800, Joe Perches wrote: > > > checkpatch is just a guide, it's brainless. > > > > > > The reason these lines are > 80 columns is > > > overly long/verbose identifiers. > > > > > > If you really want to clean up the code here, > > > the P90211ENUM_ prefixes are a bit misleading > > > as they all are #define and not enums at all. > > > > > But would you like me to remove this prefixes for all of the enums from > > p80211types.h? Are you sure it won't cause any symbol conflicts? > > No, I don't want that, I'd prefer you think about it. > > Also, a useful effort would be to (from the README) > > TODO: > [] > - move to use the in-kernel wireless stack > > where most all of the P80211 code would be removed. > > Anyway, sure, use checkpatch as a tool to help when > learning the process of how to submit patches properly. > Then move on to more thoroughly understand a block of > code in the kernel that can be improved with cleaner > style and logic and bug fixes you could submit. > > That's be much more appreciated than random 80 column > fixups and strict checkpatch compliance done without > a thorough understanding of the code. > > In any case, welcome, hope you stick around. It's clear that there are a lot more usefull patches than 80 column fix, but as you said, I learn how to submit patches and there is is constraint to submit only one checkpatch warning fix. Many thanks for advice how to proceed. I will take a look at it when I finish Greg's tutorial. Thank you, I hope to have a significant contribution to Linux Community. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-01-31 12:28 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-30 16:31 [PATCH 001] staging: wlan-ng: Add tabstop preceding the statement Maksymilian Piechota 2017-01-30 16:44 ` Greg Kroah-Hartman 2017-01-31 4:00 ` Joe Perches 2017-01-31 11:04 ` Maksymilian Piechota 2017-01-31 11:18 ` Joe Perches 2017-01-31 11:33 ` Maksymilian Piechota 2017-01-31 12:07 ` Joe Perches 2017-01-31 12:20 ` Maksymilian Piechota
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.