Solving checkpatch error with lex

DISCUSSION karthik nayak 4 participants 9 messages Dec 17, 2014 – Dec 18, 2014

Share & tools

karthik nayak

author

2014-12-17 18:34 UTC #0 lore raw

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
-------------- next part --------------
%{
#include <stdio.h>
%}

%%
"//".* {fprintf(yyout, "/*%s*/", yytext + 2);}
(.|\n) {fprintf(yyout, "%s", yytext);}
%%

main()

{
	yyin = fopen("tmp.c", "r");
	yyout = fopen("aaa.c", "w");
	yylex();
	fclose(yyin);
	fclose(yyout);
}

yywrap()

{
	return 1;
}

Valdis.Kletnieks at vt.edu

2014-12-17 18:52 UTC #1 lore raw

Quoted 1 line · karthik nayak

"//".* {fprintf(yyout, "/*%s*/", yytext + 2);}

This fails to fix block comments:

// this is line one
// this is line two
// this is line three

to
/*
* THis is line one
* This is line two
* this is line three
*/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20141217/defaafbd/attachment.bin

Paul Bolle

2014-12-17 19:07 UTC #2 lore raw

In reply to karthik nayak

Quoted 8 lines · karthik nayak

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

karthik nayak

author

2014-12-17 19:10 UTC #3 lore raw

On Thu, Dec 18, 2014, 12:37 AM Paul Bolle <[email protected]> wrote:

Quoted 6 lines · karthik nayak

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.

Quoted 2 lines

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20141217/34c4c33a/attachment.html

Greg KH

2014-12-17 19:36 UTC #4 lore raw

Quoted 22 lines · karthik nayak

On Thu, Dec 18, 2014, 12:37 AM?Paul Bolle <[email protected]> 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,

Quoted 8 lines

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.

Valdis.Kletnieks at vt.edu

2014-12-17 21:24 UTC #5 lore raw

In reply to karthik nayak

Quoted 2 lines · karthik nayak

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

tmp/z99 2014-12-17 16:22:42.831566630 -0500 renamed from staging/rtl8192u/r8192U_dm.c 2014-11-20 16:18:35.941071156 -0500 +3 -4

@@ -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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20141217/44410b83/attachment.bin

Greg KH

2014-12-18 00:47 UTC #6 lore raw

Quoted 34 lines · Valdis.Kletnieks at vt.edu

Quoted 2 lines · karthik nayak
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
tmp/z99 2014-12-17 16:22:42.831566630 -0500 renamed from staging/rtl8192u/r8192U_dm.c 2014-11-20 16:18:35.941071156 -0500 +3 -4
@@ -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 ?

Quoted 2 lines

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

karthik nayak

author

2014-12-18 06:07 UTC #7 lore raw

Thanks for your input.

Quoted 49 lines · Greg KH

Quoted 6 lines · Valdis.Kletnieks at vt.edu
Quoted 2 lines · karthik nayak
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
Quoted 5 lines
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
Quoted 2 lines
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
Quoted 1 line
0 [/usr/src/linux-next] diff -u drivers/staging/rtl8192u/r8192U_dm.c
/tmp/z99 | head -20
Quoted 1 line
--- drivers/staging/rtl8192u/r8192U_dm.c 2014-11-20
16:18:35.941071156 -0500
tmp/z99 2014-12-17 16:22:42.831566630 -0500 line 23
Quoted 5 lines
+++ /tmp/z99 2014-12-17 16:22:42.831566630 -0500
tmp/z99 2014-12-17 16:22:42.831566630 -0500 renamed from staging/rtl8192u/r8192U_dm.c 2014-11-20 16:18:35.941071156 -0500 +0 -0
@@ -21,9 +21,9 @@
 #include "r8190_rtl8256.h"
 #include "r819xU_cmdpkt.h"
 /*---------------------------Define Local
Constant---------------------------*/
tmp/z99 2014-12-17 16:22:42.831566630 -0500
Quoted 8 lines
tmp/z99 2014-12-17 16:22:42.831566630 -0500 renamed from staging/rtl8192u/r8192U_dm.c 2014-11-20 16:18:35.941071156 -0500 +3 -3
-//
-// 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};
tmp/z99 2014-12-17 16:22:42.831566630 -0500 line 38
Quoted 5 lines
tmp/z99 2014-12-17 16:22:42.831566630 -0500 renamed from staging/rtl8192u/r8192U_dm.c 2014-11-20 16:18:35.941071156 -0500 +0 -0
 static u32 edca_setting_UL[HT_IOT_PEER_MAX] =
@@ -36,11 +36,11 @@
 
 
 /*------------------------Define global variable----------------------
-------*/
Quoted 2 lines
tmp/z99 2014-12-17 16:22:42.831566630 -0500 renamed from staging/rtl8192u/r8192U_dm.c 2014-11-20 16:18:35.941071156 -0500 +0 -1
-// Debug variable ?
0 [/usr/src/linux-next] diff -u drivers/staging/rtl8192u/r8192U_dm.c
/tmp/z99 | wc -l
Quoted 1 line
1960
What a mess, I'll gladly reject a patch that does this.

Quoted 8 lines

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20141218/b37600be/attachment.html

Paul Bolle

2014-12-17 19:11 UTC #8 lore raw

In reply to Paul Bolle

Quoted 2 lines · Paul Bolle

I see no reason to review someone's lex code. I guess very few people are actually willing to do that.

My statement is not incorrect just because Valdis did actually review your code!

Paul Bolle