Submitted a first patch and no reply
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 <gajerashirish@gmail.com> --- drivers/staging/skein/skein_block.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/staging/skein/skein_block.c b/drivers/staging/skein/skein_block.c index 66261ab..5f46cd6 100644 --- a/drivers/staging/skein/skein_block.c +++ b/drivers/staging/skein/skein_block.c @@ -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
On Thu, 08 Jan 2015 12:00:07 -0800, shirish gajera said:
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 <gajerashirish@gmail.com> --- drivers/staging/skein/skein_block.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/skein/skein_block.c b/drivers/staging/skein/skein_block.c index 66261ab..5f46cd6 100644 --- a/drivers/staging/skein/skein_block.c +++ b/drivers/staging/skein/skein_block.c @@ -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)
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 On Thu, Jan 8, 2015 at 12:32 PM, <Valdis.Kletnieks@vt.edu> wrote:
On Thu, 08 Jan 2015 12:00:07 -0800, shirish gajera said:
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 <gajerashirish@gmail.com> --- drivers/staging/skein/skein_block.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/skein/skein_block.c b/drivers/staging/skein/skein_block.c index 66261ab..5f46cd6 100644 --- a/drivers/staging/skein/skein_block.c +++ b/drivers/staging/skein/skein_block.c @@ -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)
On Thu, 08 Jan 2015 12:37:15 -0800, shirish gajera said:
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?
On Thu, 2015-01-08 at 15:49 -0500, Valdis.Kletnieks@vt.edu wrote:
On Thu, 08 Jan 2015 12:37:15 -0800, shirish gajera said:
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?
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.
I will make necessary and try to re- submit the patch. On Thu, Jan 8, 2015 at 1:14 PM, Joe Perches <joe@perches.com> wrote:
On Thu, 2015-01-08 at 15:49 -0500, Valdis.Kletnieks@vt.edu wrote:
On Thu, 08 Jan 2015 12:37:15 -0800, shirish gajera said:
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?
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.
Convert all the comments to spaces before testing for single statement macros. Reported-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu> Signed-off-by: Joe Perches <joe@perches.com> ---
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(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 6afc24b..6ac355e 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -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;
Hi! On 12:00 Thu 08 Jan , shirish gajera wrote:
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". ... -#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 -- programing a layer 3+4 network protocol for mesh networks see http://michaelblizek.twilightparadox.com
participants (4)
-
Joe Perches -
michi1＠michaelblizek.twilightparadox.com -
shirish gajera -
Valdis.Kletnieks＠vt.edu