Hello, was trying to fix the checkpatch error of not using c99 comments in the file drivers/staging/rtl8192u/r8192U_dm.c so I wrote a lex program to do so since there were way too many to do it manually. Could anyone have a look at my lex code and tell me if I need to change it. I have attached the lex code. As per my testing it works perfectly, would love a second opinion. Thanks, Karthik
On Thu, 2014-12-18 at 00:04 +0530, karthik nayak wrote:
was trying to fix the checkpatch error of not using c99 comments in the file drivers/staging/rtl8192u/r8192U_dm.c so I wrote a lex program to do so since there were way too many to do it manually. Could anyone have a look at my lex code and tell me if I need to change it. I have attached the lex code. As per my testing it works perfectly, would love a second opinion.
I see no reason to review someone's lex code. I guess very few people are actually willing to do that. Why don't you just send in the diff? Or even better, the patch you intend to submit. Almost everybody here can read diffs or patches. Hope this helps, Paul Bolle
On Thu, Dec 18, 2014, 12:37 AM Paul Bolle <pebolle@tiscali.nl> wrote: On Thu, 2014-12-18 at 00:04 +0530, karthik nayak wrote:
was trying to fix the checkpatch error of not using c99 comments in the file drivers/staging/rtl8192u/r8192U_dm.c so I wrote a lex program to do so since there were way too many to do it manually. Could anyone have a look at my lex code and tell me if I need to change it. I have attached the lex code. As per my testing it works perfectly, would love a second opinion.
I see no reason to review someone's lex code. I guess very few people are actually willing to do that. Why don't you just send in the diff? Or even better, the patch you intend to submit. Almost everybody here can read diffs or patches. Hope this helps, Paul Bolle Hello Paul, The only reason I didn't do that is cause the file is too large to manually have a look at, even the diff tends to be very huge. But if you think that would help, I wouldn't mind attaching the diff. Thanks Karthik
On Wed, Dec 17, 2014 at 07:10:24PM +0000, karthik nayak wrote:
On Thu, Dec 18, 2014, 12:37 AM Paul Bolle <pebolle@tiscali.nl> wrote:
On Thu, 2014-12-18 at 00:04 +0530, karthik nayak wrote: > was trying to fix the checkpatch error of not using c99 comments in the > file > drivers/staging/rtl8192u/r8192U_dm.c > so I wrote a lex program to do so since there were way too many to do it > manually. > Could anyone have a look at my lex code and tell me if I need to change it. > I have attached the lex code. > As per my testing it works perfectly, would love a second opinion.
I see no reason to review someone's lex code. I guess very few people are actually willing to do that.
Why don't you just send in the diff? Or even better, the patch you intend to submit. Almost everybody here can read diffs or patches.
Hope this helps,
Paul Bolle
Hello Paul, The only reason I didn't do that is cause the file is too large to manually have a look at, even the diff tends to be very huge.
Then the diff will not be accepted upstream either if it is too big to review.
On Wed, 17 Dec 2014 19:10:24 +0000, karthik nayak said:
The only reason I didn't do that is cause the file is too large to manually have a look at, even the diff tends to be very huge.
[/usr/src/linux-next] grep '//' drivers/staging/rtl8192u/r8192U_dm.c | wc -l 373 And why use lex when sed is up to the task? [/usr/src/linux-next] sed 's?//(.*)$?/* \1 */?' drivers/staging/rtl8192u/r8192U_dm.c >| /tmp/z99 sed: -e expression #1, char 19: invalid reference \1 on `s' command's RHS 1 [/usr/src/linux-next] sed 's?//\(.*\)$?/* \1 */?' drivers/staging/rtl8192u/r8192U_dm.c >| /tmp/z99 0 [/usr/src/linux-next] diff -u drivers/staging/rtl8192u/r8192U_dm.c /tmp/z99 | head -20 --- drivers/staging/rtl8192u/r8192U_dm.c 2014-11-20 16:18:35.941071156 -0500 +++ /tmp/z99 2014-12-17 16:22:42.831566630 -0500 @@ -21,9 +21,9 @@ #include "r8190_rtl8256.h" #include "r819xU_cmdpkt.h" /*---------------------------Define Local Constant---------------------------*/ -// -// Indicate different AP vendor for IOT issue. -// +/* */ +/* Indicate different AP vendor for IOT issue. */ +/* */ static u32 edca_setting_DL[HT_IOT_PEER_MAX] = { 0x5e4322, 0x5e4322, 0x5e4322, 0x604322, 0xa44f, 0x5ea44f}; static u32 edca_setting_UL[HT_IOT_PEER_MAX] = @@ -36,11 +36,11 @@ /*------------------------Define global variable-----------------------------*/ -// Debug variable ? 0 [/usr/src/linux-next] diff -u drivers/staging/rtl8192u/r8192U_dm.c /tmp/z99 | wc -l 1960 2K lines isn't that hard to review, if the *only* thing you do is fix c99 comments. I'd recommend it as *two* patches - one to do basically what your lex code does and convert '// foo bar baz' to '/* foo bar baz */' And then a *second* patch to fix block comments to /* * */ form.
On Wed, Dec 17, 2014 at 04:24:15PM -0500, Valdis.Kletnieks@vt.edu wrote:
On Wed, 17 Dec 2014 19:10:24 +0000, karthik nayak said:
The only reason I didn't do that is cause the file is too large to manually have a look at, even the diff tends to be very huge.
[/usr/src/linux-next] grep '//' drivers/staging/rtl8192u/r8192U_dm.c | wc -l 373
And why use lex when sed is up to the task?
[/usr/src/linux-next] sed 's?//(.*)$?/* \1 */?' drivers/staging/rtl8192u/r8192U_dm.c >| /tmp/z99 sed: -e expression #1, char 19: invalid reference \1 on `s' command's RHS 1 [/usr/src/linux-next] sed 's?//\(.*\)$?/* \1 */?' drivers/staging/rtl8192u/r8192U_dm.c >| /tmp/z99 0 [/usr/src/linux-next] diff -u drivers/staging/rtl8192u/r8192U_dm.c /tmp/z99 | head -20 --- drivers/staging/rtl8192u/r8192U_dm.c 2014-11-20 16:18:35.941071156 -0500 +++ /tmp/z99 2014-12-17 16:22:42.831566630 -0500 @@ -21,9 +21,9 @@ #include "r8190_rtl8256.h" #include "r819xU_cmdpkt.h" /*---------------------------Define Local Constant---------------------------*/ -// -// Indicate different AP vendor for IOT issue. -// +/* */ +/* Indicate different AP vendor for IOT issue. */ +/* */ static u32 edca_setting_DL[HT_IOT_PEER_MAX] = { 0x5e4322, 0x5e4322, 0x5e4322, 0x604322, 0xa44f, 0x5ea44f}; static u32 edca_setting_UL[HT_IOT_PEER_MAX] = @@ -36,11 +36,11 @@
/*------------------------Define global variable-----------------------------*/ -// Debug variable ? 0 [/usr/src/linux-next] diff -u drivers/staging/rtl8192u/r8192U_dm.c /tmp/z99 | wc -l 1960
What a mess, I'll gladly reject a patch that does this. Look at that multi-line crud. ick ick ick, don't run auto-formatting tools just to fix up checkpatch issues. If that was all that is needed, I would have done that a long time ago, that's not the goal here people... greg k-h
Thanks for your input. On Thu, Dec 18, 2014, 6:17 AM Greg KH <greg@kroah.com> wrote:
On Wed, Dec 17, 2014 at 04:24:15PM -0500, Valdis.Kletnieks@vt.edu wrote:
On Wed, 17 Dec 2014 19:10:24 +0000, karthik nayak said:
The only reason I didn't do that is cause the file is too large to manually have a look at, even the diff tends to be very huge.
[/usr/src/linux-next] grep '//' drivers/staging/rtl8192u/r8192U_dm.c | wc -l 373
And why use lex when sed is up to the task?
[/usr/src/linux-next] sed 's?//(.*)$?/* \1 */?' drivers/staging/rtl8192u/r8192U_dm.c >| /tmp/z99 sed: -e expression #1, char 19: invalid reference \1 on `s' command's RHS 1 [/usr/src/linux-next] sed 's?//\(.*\)$?/* \1 */?' drivers/staging/rtl8192u/r8192U_dm.c >| /tmp/z99 0 [/usr/src/linux-next] diff -u drivers/staging/rtl8192u/r8192U_dm.c /tmp/z99 | head -20 --- drivers/staging/rtl8192u/r8192U_dm.c 2014-11-20 16:18:35.941071156 -0500 +++ /tmp/z99 2014-12-17 16:22:42.831566630 -0500 @@ -21,9 +21,9 @@ #include "r8190_rtl8256.h" #include "r819xU_cmdpkt.h" /*---------------------------Define Local Constant---------------------------*/ -// -// Indicate different AP vendor for IOT issue. -// +/* */ +/* Indicate different AP vendor for IOT issue. */ +/* */ static u32 edca_setting_DL[HT_IOT_PEER_MAX] = { 0x5e4322, 0x5e4322, 0x5e4322, 0x604322, 0xa44f, 0x5ea44f}; static u32 edca_setting_UL[HT_IOT_PEER_MAX] = @@ -36,11 +36,11 @@
/*------------------------Define global variable---------------------- -------*/ -// Debug variable ? 0 [/usr/src/linux-next] diff -u drivers/staging/rtl8192u/r8192U_dm.c /tmp/z99 | wc -l 1960
What a mess, I'll gladly reject a patch that does this.
Look at that multi-line crud.
ick ick ick, don't run auto-formatting tools just to fix up checkpatch issues. If that was all that is needed, I would have done that a long time ago, that's not the goal here people...
greg k-h
participants (4)
-
Greg KH -
karthik nayak -
Paul Bolle -
Valdis.Kletnieks＠vt.edu