Submitted a first patch and no reply

PATCH New shirish gajera 4 participants 8 messages Jan 8, 2015 – Jan 9, 2015

Subsystems (2)

Staging Checkpatch

lore

Share & tools

lore

shirish gajera

author

2015-01-08 20:00 UTC #0 lore raw

Hi,

I have submitted my first patch on 12/30/2014 and I have not received any reply for next step like error or merge ect.

I send the email to person that get_maintainer.pl script tells me to send.

Please guide whom should I reached for this.

Below is the email
---------
This patch fixes the checkpatch.pl warning:

WARNING: Single statement macros should not use a do {} while (0) loop

I have added single statement in curly braces, because it was giving me "WARNING: macros should not use a trailing semicolon".

Signed-off-by Shirish Gajera <[email protected]>

---
 drivers/staging/skein/skein_block.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/staging/skein/skein_block.c
index 66261ab..5f46cd6 100644

drivers/staging/skein/skein_block.c +4 -4

@@ -82,10 +82,10 @@ do {
                  \
 } while (0)
 #else
 /* looping version */
-#define R256(p0, p1, p2, p3, ROT, r_num) \
-do { \
-       ROUND256(p0, p1, p2, p3, ROT, r_num); \
-} while (0)
+#define R256(p0, p1, p2, p3, ROT, r_num)        \
+{                                              \
+       ROUND256(p0, p1, p2, p3, ROT, r_num);   \
+}
 
 #define I256(R) \

do { \

---------------------------

Thanks,
Shirish
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150108/63ccf957/attachment.html

Valdis.Kletnieks at vt.edu

2015-01-08 20:32 UTC #1 lore raw

Quoted 16 lines · shirish gajera

WARNING: Single statement macros should not use a do {} while (0) loop
I have added single statement in curly braces, because it was giving me "WARNING: macros should not use a trailing semicolon".
Signed-off-by Shirish Gajera <[email protected]>
---
 drivers/staging/skein/skein_block.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/staging/skein/skein_block.c +0 -0
index 66261ab..5f46cd6 100644

drivers/staging/skein/skein_block.c do { · line 86

Quoted 5 lines

drivers/staging/skein/skein_block.c +0 -1

@@ -82,10 +82,10 @@ do {
                  \
 } while (0)
 #else
 /* looping version */
-#define R256(p0, p1, p2, p3, ROT, r_num) \

Why did you not fix the definition of R256 in the other half of the "#if SKEIN_UNROLL_256 == 0" code? Or the definitions for R512 and R1024? You cleaned up one case, and left another 5 cases alone....

(This is part of why blindly fixing checkpatch complaints without actually looking at the code and understanding isn't always a good thing)

-------------- 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/20150108/c058b11b/attachment.bin

shirish gajera

author

2015-01-08 20:37 UTC #2 lore raw

Hi,

Actually on the website it's return that

Pick a warning, and try to fix it. For your first patch, only pick one warning. In the future you can group multiple changes into one patch, but only if you follow the PatchPhilosophy
<http://kernelnewbies.org/PatchPhilosophy> of breaking each patch into logical changes.

That's hwy I just fix one warning.

Please let me know if I can fix more than one warning in my first patch.

Thanks,

Shirish

Quoted 28 lines · On Thu, Jan 8, 2015 at 12:32 PM, <[email protected]> wrote:

Quoted 16 lines · shirish gajera
WARNING: Single statement macros should not use a do {} while (0) loop
I have added single statement in curly braces, because it was giving me "WARNING: macros should not use a trailing semicolon".
Signed-off-by Shirish Gajera <[email protected]>
---
 drivers/staging/skein/skein_block.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/staging/skein/skein_block.c +0 -0
index 66261ab..5f46cd6 100644
drivers/staging/skein/skein_block.c do { · line 86
Quoted 5 lines
drivers/staging/skein/skein_block.c +0 -1
@@ -82,10 +82,10 @@ do {
                  \
 } while (0)
 #else
 /* looping version */
-#define R256(p0, p1, p2, p3, ROT, r_num) \
Why did you not fix the definition of R256 in the other half of the "#if SKEIN_UNROLL_256 == 0" code? Or the definitions for R512 and R1024? You cleaned up one case, and left another 5 cases alone....

Quoted 4 lines

(This is part of why blindly fixing checkpatch complaints without actually looking at the code and understanding isn't always a good thing)

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150108/1b171d6d/attachment.html

Valdis.Kletnieks at vt.edu

2015-01-08 20:49 UTC #3 lore raw

Quoted 1 line · shirish gajera

That's hwy I just fix one warning.

That means "don't fix a warning about indentation *and* a warning about trailing blanks in the same patch".

Also, if you're fixing a style issue, you should actually *review* the code, and make sure you fix the issue everywhere, *even if the other ones didn't prompt a warning*.

It's interesting that the warning only popped on the one case where the "/* looping version */" is on a separate line, not on the #else line.

Am cc'ing Joe Perches for that question. Joe - to get you up to speed,
checkpatch was run against drivers/staging/skein/skein_block.c, and it
flagged one definition of the macro R256, but not the other one, or the
two definitions each for R512 and R1024. Any idea why?
-------------- 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/20150108/aee60739/attachment.bin

Joe Perches

maintainer

2015-01-08 21:14 UTC #4 lore raw

Quoted 11 lines · Valdis.Kletnieks at vt.edu

Quoted 1 line · shirish gajera
That's hwy I just fix one warning.
That means "don't fix a warning about indentation *and* a warning about trailing blanks in the same patch".
Also, if you're fixing a style issue, you should actually *review* the code, and make sure you fix the issue everywhere, *even if the other ones didn't prompt a warning*.

Quoted 7 lines

It's interesting that the warning only popped on the one case where the "/* looping version */" is on a separate line, not on the #else line.
Am cc'ing Joe Perches for that question. Joe - to get you up to speed, checkpatch was run against drivers/staging/skein/skein_block.c, and it flagged one definition of the macro R256, but not the other one, or the two definitions each for R512 and R1024. Any idea why?

I believe the comment in the macro interferes with the perl regex.

        #if SKEIN_UNROLL_256 == 0
        #define R256(p0, p1, p2, p3, ROT, r_num) /* fully unrolled */ \
        do {                                                          \
        	ROUND256(p0, p1, p2, p3, ROT, r_num);                 \
        } while (0)

With the latest version in -next I get:

$ ./scripts/checkpatch.pl -f --strict drivers/staging/skein/skein_block.c --types=SINGLE_STATEMENT_DO_WHILE_MACRO
WARNING: Single statement macros should not use a do {} while (0) loop
#85: FILE: drivers/staging/skein/skein_block.c:85:
+#define R256(p0, p1, p2, p3, ROT, r_num) \
+do { \
+	ROUND256(p0, p1, p2, p3, ROT, r_num); \
+} while (0)

WARNING: Single statement macros should not use a do {} while (0) loop #176: FILE: drivers/staging/skein/skein_block.c:176:
+#define R512(p0, p1, p2, p3, p4, p5, p6, p7, ROT, r_num) \
+do { \
+ ROUND512(p0, p1, p2, p3, p4, p5, p6, p7, ROT, r_num); \
+} while (0)

WARNING: Single statement macros should not use a do {} while (0) loop #264: FILE: drivers/staging/skein/skein_block.c:264:
+#define R1024(p0, p1, p2, p3, p4, p5, p6, p7, p8, p9, pA, pB, pC, pD, pE, pF, \
+ ROT, rn) \
+do { \
+ ROUND1024(p0, p1, p2, p3, p4, p5, p6, p7, p8, p9, pA, pB, pC, pD, pE, \
+ pF, ROT, rn); \
+} while (0)

WARNING: Single statement macros should not use a do {} while (0) loop #292: FILE: drivers/staging/skein/skein_block.c:292:
+#define R1024(p0, p1, p2, p3, p4, p5, p6, p7, p8, p9, pA, pB, pC, pD, pE, pF, \
+ ROT, rn) \
+do { \
+ ROUND1024(p0, p1, p2, p3, p4, p5, p6, p7, p8, p9, pA, pB, pC, pD, pE, \
+ pF, ROT, rn); \
+} while (0)

total: 0 errors, 4 warnings, 0 checks, 795 lines checked

NOTE: Used message types: SINGLE_STATEMENT_DO_WHILE_MACRO

drivers/staging/skein/skein_block.c has style problems, please review.

If any of these errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.

shirish gajera

author

2015-01-08 21:25 UTC #5 lore raw

I will make necessary and try to re- submit the patch.

Quoted 75 lines · Joe Perches

Quoted 11 lines · Valdis.Kletnieks at vt.edu
Quoted 1 line · shirish gajera
That's hwy I just fix one warning.
That means "don't fix a warning about indentation *and* a warning about trailing blanks in the same patch".
Also, if you're fixing a style issue, you should actually *review* the code, and make sure you fix the issue everywhere, *even if the other ones didn't prompt a warning*.
Quoted 7 lines
It's interesting that the warning only popped on the one case where the "/* looping version */" is on a separate line, not on the #else line.
Am cc'ing Joe Perches for that question. Joe - to get you up to speed, checkpatch was run against drivers/staging/skein/skein_block.c, and it flagged one definition of the macro R256, but not the other one, or the two definitions each for R512 and R1024. Any idea why?
I believe the comment in the macro interferes with the perl regex.
        #if SKEIN_UNROLL_256 == 0
        #define R256(p0, p1, p2, p3, ROT, r_num) /* fully unrolled */ \
        do {                                                          \
                ROUND256(p0, p1, p2, p3, ROT, r_num);                 \
        } while (0)
With the latest version in -next I get:
$ ./scripts/checkpatch.pl -f --strict drivers/staging/skein/skein_block.c
--types=SINGLE_STATEMENT_DO_WHILE_MACRO
WARNING: Single statement macros should not use a do {} while (0) loop
#85: FILE: drivers/staging/skein/skein_block.c:85:
drivers/staging/skein/skein_block.c +4 -0
+#define R256(p0, p1, p2, p3, ROT, r_num) \
+do { \
+       ROUND256(p0, p1, p2, p3, ROT, r_num); \
+} while (0)
WARNING: Single statement macros should not use a do {} while (0) loop #176: FILE: drivers/staging/skein/skein_block.c:176:
(quoted diff) +4 -0
+#define R512(p0, p1, p2, p3, p4, p5, p6, p7, ROT, r_num)                 \
+do {                                                                     \
+       ROUND512(p0, p1, p2, p3, p4, p5, p6, p7, ROT, r_num);            \
+} while (0)
WARNING: Single statement macros should not use a do {} while (0) loop #264: FILE: drivers/staging/skein/skein_block.c:264:
+#define R1024(p0, p1, p2, p3, p4, p5, p6, p7, p8, p9, pA, pB, pC, pD, pE, pF, \
(quoted diff) +3 -0
+             ROT, rn)
    \
+do {
    \
+       ROUND1024(p0, p1, p2, p3, p4, p5, p6, p7, p8, p9, pA, pB, pC, pD,
pE, \
(quoted diff) +2 -0
+                 pF, ROT, rn);
   \
+} while (0)
WARNING: Single statement macros should not use a do {} while (0) loop #292: FILE: drivers/staging/skein/skein_block.c:292:
+#define R1024(p0, p1, p2, p3, p4, p5, p6, p7, p8, p9, pA, pB, pC, pD, pE, pF, \
(quoted diff) +3 -0
+             ROT, rn)
    \
+do {
    \
+       ROUND1024(p0, p1, p2, p3, p4, p5, p6, p7, p8, p9, pA, pB, pC, pD,
pE, \
(quoted diff) +2 -0
+                 pF, ROT, rn);
   \
+} while (0)

Quoted 11 lines

total: 0 errors, 4 warnings, 0 checks, 795 lines checked
NOTE: Used message types: SINGLE_STATEMENT_DO_WHILE_MACRO
drivers/staging/skein/skein_block.c has style problems, please review.
If any of these errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150108/6b72e29e/attachment-0001.html

Joe Perches

maintainer

2015-01-08 21:31 UTC #6 lore raw

PATCH 1/1 checkpatch: Allow comments in macros tested for single statements

In reply to Valdis.Kletnieks at vt.edu

Convert all the comments to spaces before testing for single statement macros.

Reported-by Valdis Kletnieks <[email protected]>

Signed-off-by Joe Perches <[email protected]>

---

Quoted 7 lines

It's interesting that the warning only popped on the one case where the "/* looping version */" is on a separate line, not on the #else line.
Am cc'ing Joe Perches for that question. Joe - to get you up to speed, checkpatch was run against drivers/staging/skein/skein_block.c, and it flagged one definition of the macro R256, but not the other one, or the two definitions each for R512 and R1024. Any idea why?

Yup, that was it...

 scripts/checkpatch.pl | 1 +
 1 file changed, 1 insertion(+)

scripts/checkpatch.pl +1 -0

index 6afc24b..6ac355e 100755
@@ -4260,6 +4260,7 @@ sub process {
 			$ctx = $dstat;
 
 			$dstat =~ s/\\\n.//g;
+			$dstat =~ s/$;/ /g;
 
 			if ($dstat =~ /^\+\s*#\s*define\s+$Ident\s*${balanced_parens}\s*do\s*{(.*)\s*}\s*while\s*\(\s*0\s*\)\s*([;\s]*)\s*$/) {
 				my $stmts = $2;

michi1 at michaelblizek.twilightparadox.com

2015-01-09 16:55 UTC #7 lore raw

In reply to shirish gajera

Hi!

Quoted 6 lines · shirish gajera

This patch fixes the checkpatch.pl warning:
WARNING: Single statement macros should not use a do {} while (0) loop
I have added single statement in curly braces, because it was giving me "WARNING: macros should not use a trailing semicolon".

...

drivers/staging/skein/skein_block.c do { · line 89

Quoted 8 lines

drivers/staging/skein/skein_block.c +4 -4

-#define R256(p0, p1, p2, p3, ROT, r_num) \
-do { \
-       ROUND256(p0, p1, p2, p3, ROT, r_num); \
-} while (0)
+#define R256(p0, p1, p2, p3, ROT, r_num)        \
+{                                              \
+       ROUND256(p0, p1, p2, p3, ROT, r_num);   \
+}

This is wrong. Take a look at the reason of the do...while:
http://gcc.gnu.org/onlinedocs/cpp/Swallowing-the-Semicolon.html

You probably want to replace it with something like this:
#define R256(p0, p1, p2, p3, ROT, r_num) ROUND256(p0, p1, p2, p3, ROT, r_num) or maybe even convert it to an inline function.

-Michi

Signature · 2 lines

programing a layer 3+4 network protocol for mesh networks
see http://michaelblizek.twilightparadox.com