You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "LuchianMihai (via GitHub)" <gi...@apache.org> on 2025/02/15 17:44:07 UTC

[PR] tools/nxstyle: handle case statement [nuttx]

LuchianMihai opened a new pull request, #15847:
URL: https://github.com/apache/nuttx/pull/15847

   ## Summary
   
   Simple fix for [15838](https://github.com/apache/nuttx/issues/15838)
   *Note:* The comments are also treated as statement:
   ```c
   case ':': /* error is thrown here */
     {
       do something
     }
   ```
   Can solve that corner case if needed also.
   
   ## Impact
   
   New style errors can be reported throughout the codebase.
   
   ## Testing
   
   Localy, running nxstyle over a wrongly formatted file.
   If case statement continues on the same line an error will be generated
   ```
   ~/Projects/nxspace/nuttx/tools/nxstyle.c:1203:14: error: Case statement should be on a new line
   ```
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#issuecomment-2663331624

   @LuchianMihai thank you for fixing the issue!
   
   Question: Is the new code able to keep catching line with C++ comment:
   
   case x: // C++ comment not allowed


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#issuecomment-2672007192

   I have a question about this one:
   
   ```
   /work/NuttX/nuttx/arch/arm/src/cxd56xx/cxd56_dmac.c:316:10: error: Case statement should be on a new line
   ```
   
   This is when a switch looks like this:
   
   ```
   static int ch2dmac(int ch)
   {
     switch (ch)
       {
       case 0: case 1:
           return 1;
       case 2: case 3: case 4: case 5: case 6: /* APP IDMAC */
           return 3;
       case 7: case 8: /* APP SKDMAC */
           return 2;
       default:
           return 0;
       }
   }
   ```
   
   Do we want to allow this shorthand format (like `case 0: case 1:`) or do we want to force each case on a separate line?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on code in PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#discussion_r1957369051


##########
tools/nxstyle.c:
##########
@@ -2042,6 +2033,17 @@ int main(int argc, char **argv, char **envp)
             {
               bswitch = true;
             }
+          else if (strncmp(&line[indent], "switch(", 7) == 0)
+            {
+              ERROR("Missing whitespace after keyword", lineno, n);
+
+              bswitch = true;
+            }
+          else if (strncmp(&line[indent], "case ", 5) == 0 ||
+                   strncmp(&line[indent], "default ", 8) == 0)

Review Comment:
   Good point @hartmannathan, actually I found "default :" as well on nuttx code, but there are few cases, we can fix it and use "default:" as default :-) Or we can accept both, that do you thing @LuchianMihai ?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "LuchianMihai (via GitHub)" <gi...@apache.org>.
LuchianMihai commented on PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#issuecomment-2672740091

   But the switch issue should be solved


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#issuecomment-2671970946

   OK the very first new report is:
   
   ```
   /work/NuttX/nuttx/drivers/coresight/coresight_etm4.c:618:6: error: Unexpected whitespace found after keyword
   ```
   
   which is:
   
   ```
   static uint64_t etm4_sysreg_read(uint32_t offset)
   {
     uint64_t res = 0;
   
     switch (offset)
       {
         ETM4_READ_SYSREG_CASES(res)
         default :
           cserr("etm4x: trying to read unsupported register @%x\n", offset);
       }
   
     return res;
   }
   ```
   
   which indeed has a spurious space after `default`, so nxstyle is giving a valid new report here!
   
   I guess a separate PR can go and fix these...


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on code in PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#discussion_r1957367166


##########
tools/nxstyle.c:
##########
@@ -2042,6 +2033,17 @@ int main(int argc, char **argv, char **envp)
             {
               bswitch = true;
             }
+          else if (strncmp(&line[indent], "switch(", 7) == 0)
+            {
+              ERROR("Missing whitespace after keyword", lineno, n);
+
+              bswitch = true;
+            }
+          else if (strncmp(&line[indent], "case ", 5) == 0 ||
+                   strncmp(&line[indent], "default ", 8) == 0)

Review Comment:
   Is `default` keyword supposed to be followed by space character here? Usually it is written `default:` (with colon).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#issuecomment-2671976594

   Uh oh... there's a little problem:
   
   ```
   /work/NuttX/nuttx/sched/sched/sched_reprioritizertr.c:67:2: error: Missing whitespace after keyword
   /work/NuttX/nuttx/sched/sched/sched_reprioritizertr.c:81:2: error: Missing whitespace after keyword
   ```
   
   Here, it's incorrectly picking up the substring `switch` in a variable called `switch_needed`:
   
   ```
   bool nxsched_reprioritize_rtr(FAR struct tcb_s *tcb, int priority)
   {
     bool switch_needed;
   
     /* Remove the tcb task from the ready-to-run list.
      * nxsched_remove_readytorun will return true if we just
      * remove the head of the ready to run list.
      */
   
     switch_needed = nxsched_remove_readytorun(tcb);
   
     /* Setup up the new task priority */
   
     tcb->sched_priority = (uint8_t)priority;
   
     /* Return the task to the specified blocked task list.
      * nxsched_add_readytorun will return true if the task was
      * added to the new list.  We will need to perform a context
      * switch only if the EXCLUSIVE or of the two calls is non-zero
      * (i.e., one and only one the calls changes the head of the
      * ready-to-run list).
      */
   
     switch_needed ^= nxsched_add_readytorun(tcb);
   
     /* If we are going to do a context switch, then now is the right
      * time to add any pending tasks back into the ready-to-run list.
      */
   
     if (switch_needed && list_pendingtasks()->head)
       {
         nxsched_merge_pending();
       }
   
     return switch_needed;
   }
   ```
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#issuecomment-2672816904

   > > Do we want to allow this shorthand format (like `case 0: case 1:`) or do we want to force each case on a separate line?
   > 
   > I think two cases in the same line are not correct, probably it passed because our nxstyle wasn't able to catch this kind of thing. It is better to fix it
   
   OK, in that case, this PR has fixed it.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "LuchianMihai (via GitHub)" <gi...@apache.org>.
LuchianMihai commented on code in PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#discussion_r1957370313


##########
tools/nxstyle.c:
##########
@@ -2042,6 +2033,17 @@ int main(int argc, char **argv, char **envp)
             {
               bswitch = true;
             }
+          else if (strncmp(&line[indent], "switch(", 7) == 0)
+            {
+              ERROR("Missing whitespace after keyword", lineno, n);
+
+              bswitch = true;
+            }
+          else if (strncmp(&line[indent], "case ", 5) == 0 ||
+                   strncmp(&line[indent], "default ", 8) == 0)

Review Comment:
   I see this as an example in the documentation  
   ```c
   switch (...)
     {
       case 1:  /* Example of a comment following a case selector. */
       ...
   
       /* Example of a comment preceding a case selector. */
   
       case 2:
         {
           /* Example of comment following the case selector. */
   
           int value;
           ...
         }
         break;
   
       default:
         break;
     }
   ```
   So I guess no space after default keyword



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "LuchianMihai (via GitHub)" <gi...@apache.org>.
LuchianMihai commented on PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#issuecomment-2663526311

   > > > @LuchianMihai thank you for fixing the issue!
   > > > Question: Is the new code able to keep catching line with C++ comment:
   > > > case x: // C++ comment not allowed
   > > 
   > > 
   > > I'm not altering the comments behavior, I kinda detect comments ahead of time, using the same logic, comments start with /, but I do not alter how comments are handled. It throws error if c++ style comment is detected. Guess that is the expected behavior looking at the code.
   > 
   > Please just do a test: modify a file to include "// some comment" after "case xyz:" and run:
   > 
   > ./tools/checkpatch.sh -f path/to/modified/file.c
   
   Did that before adding previous comment


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#issuecomment-2672742681

   > I have a question about this one:
   > 
   > ```
   > /work/NuttX/nuttx/arch/arm/src/cxd56xx/cxd56_dmac.c:316:10: error: Case statement should be on a new line
   > ```
   > 
   > This is when a switch looks like this:
   > 
   > ```
   > static int ch2dmac(int ch)
   > {
   >   switch (ch)
   >     {
   >     case 0: case 1:
   >         return 1;
   >     case 2: case 3: case 4: case 5: case 6: /* APP IDMAC */
   >         return 3;
   >     case 7: case 8: /* APP SKDMAC */
   >         return 2;
   >     default:
   >         return 0;
   >     }
   > }
   > ```
   > 
   > Do we want to allow this shorthand format (like `case 0: case 1:`) or do we want to force each case on a separate line?
   
   I think two cases in the same line are not correct, probably it passed because our nxstyle wasn't able to catch this kind of thing. It is better to fix it


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#issuecomment-2672036848

   OK, I went through all the new nxstyle reports.
   
   The good news: I didn't see any regressions, meaning that errors that nxstyle reported previously are still being reported.
   
   The slightly not-so-good news: We seem to be picking up words like `switch` when they occur in a substring like `switch_needed` or `switching`.
   
   One more comment: Most of the new "Missing whitespace after keyword" errors are related to things like this:
   
   ```
   #ifdef CONFIG_PM
   static void up_pm_notify(struct pm_callback_s *cb, int domain,
                            enum pm_state_e pmstate)
   {
     switch (pmstate)
       {
         case(PM_NORMAL):
           {
             /* Logic for PM_NORMAL goes here */
           }
           break;
   
         case(PM_IDLE):
           {
             /* Logic for PM_IDLE goes here */
           }
           break;
   
         case(PM_STANDBY):
           {
             /* Logic for PM_STANDBY goes here */
           }
           break;
   
         case(PM_SLEEP):
           {
             /* Logic for PM_SLEEP goes here */
           }
           break;
   
         default:
   
           /* Should not get here */
   
           break;
       }
   }
   #endif
   ```
   
   I haven't looked further into why `PM_NORMAL` and the others are in parenthesis. Perhaps the parenthesis should be moved to the macro definitions? I don't know... that will take more research that is outside the scope of this PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "LuchianMihai (via GitHub)" <gi...@apache.org>.
LuchianMihai commented on PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#issuecomment-2672734068

   This will take a lot of work...
   I tried to avoid the following situation, but without refactoring it will not be possible.
   eg. switch should be checked explicitly for every valid syntax.
   ```c
            else if (strncmp(&line[indent], "switch ", 7) == 0)
              {
                bswitch = true;
              }
            else if (strncmp(&line[indent], "switch(", 7) == 0)
              {
                ERROR("Missing whitespace after keyword", lineno, n);
                bswitch = true;
              }
            else if (strncmp(&line[indent], "case ", 5) == 0)
              {
                bcase = true;
              }
            else if (strncmp(&line[indent], "case(", 5) == 0)
              {
                ERROR("Missing whitespace after keyword", lineno, n);
                bcase = true;
              }
            else if (strncmp(&line[indent], "default ", 8) == 0)
              {
                ERROR("Missing whitespace after keyword", lineno, n);
                bcase = true;
              }
            else if (strncmp(&line[indent], "default:", 8) == 0)
              {
                bcase = true;
              }
   ```
   So just by this example I can see that `default` keyword should be handled with one or more white spaces and colons else we face same problem if we use defaulting or similar words as part of names.  
   `break` faces similar problem, nxstyle will confuse it with breaking, breakage and others.
   
   Unfortunately nxstyle compares strings without tokenizing the file. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "LuchianMihai (via GitHub)" <gi...@apache.org>.
LuchianMihai commented on code in PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#discussion_r1957369872


##########
tools/nxstyle.c:
##########
@@ -2042,6 +2033,17 @@ int main(int argc, char **argv, char **envp)
             {
               bswitch = true;
             }
+          else if (strncmp(&line[indent], "switch(", 7) == 0)
+            {
+              ERROR("Missing whitespace after keyword", lineno, n);
+
+              bswitch = true;
+            }
+          else if (strncmp(&line[indent], "case ", 5) == 0 ||
+                   strncmp(&line[indent], "default ", 8) == 0)

Review Comment:
   Nice catch, I find them both throughout the code base.  
   I did not thought about that, both strncmp (for case and default) were already present in the nxstyle.c but thrown out with #if 0, I just copy-pasted that part.  
   Will fix this also, thank you.
     



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on code in PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#discussion_r1957369051


##########
tools/nxstyle.c:
##########
@@ -2042,6 +2033,17 @@ int main(int argc, char **argv, char **envp)
             {
               bswitch = true;
             }
+          else if (strncmp(&line[indent], "switch(", 7) == 0)
+            {
+              ERROR("Missing whitespace after keyword", lineno, n);
+
+              bswitch = true;
+            }
+          else if (strncmp(&line[indent], "case ", 5) == 0 ||
+                   strncmp(&line[indent], "default ", 8) == 0)

Review Comment:
   Good point @hartmannathan, actually I found "default  :" as well on nuttx code, but there are few cases, we can fix it and use "default:" as default :-) Or we can accept both, that do you thing @LuchianMihai ?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on code in PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#discussion_r1957442784


##########
tools/nxstyle.c:
##########
@@ -2042,6 +2033,17 @@ int main(int argc, char **argv, char **envp)
             {
               bswitch = true;
             }
+          else if (strncmp(&line[indent], "switch(", 7) == 0)
+            {
+              ERROR("Missing whitespace after keyword", lineno, n);
+
+              bswitch = true;
+            }
+          else if (strncmp(&line[indent], "case ", 5) == 0 ||
+                   strncmp(&line[indent], "default ", 8) == 0)

Review Comment:
   Yes, I think you can follow this standard! Since there are only a couple of files with "default :" it is easy to fix



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "LuchianMihai (via GitHub)" <gi...@apache.org>.
LuchianMihai commented on PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#issuecomment-2672642297

   Hi, I can fix the switch related errors quickly. Previously, there was an compare to switch that included space at the end, and another one that included an open parenthesis. I thought that I can have a simple strncmp to switch keyword and handle the white spaces after, but the space was way more important than I thought.
   
   Anyway, amazing work, I'll go through your comments in details tomorrow and come with needed fixes


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan merged PR #15847:
URL: https://github.com/apache/nuttx/pull/15847


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "LuchianMihai (via GitHub)" <gi...@apache.org>.
LuchianMihai commented on PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#issuecomment-2663380058

   > @LuchianMihai thank you for fixing the issue!
   > 
   > Question: Is the new code able to keep catching line with C++ comment:
   > 
   > case x: // C++ comment not allowed
   
   I'm not altering the comments behavior,  
   I kinda detect comments ahead of time, using the same logic, comments start with /, but I do not alter how comments are handled.
   It throws error if c++ style comment is detected. Guess that is the expected behavior looking at the code.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#issuecomment-2663512463

   > > @LuchianMihai thank you for fixing the issue!
   > > Question: Is the new code able to keep catching line with C++ comment:
   > > case x: // C++ comment not allowed
   > 
   > I'm not altering the comments behavior, I kinda detect comments ahead of time, using the same logic, comments start with /, but I do not alter how comments are handled. It throws error if c++ style comment is detected. Guess that is the expected behavior looking at the code.
   
   Please just do a test: modify a file to include // some comment after "case xyz:" and run:
   
   ./tools/checkpatch.sh -f path/to/modified/file.c


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] tools/nxstyle: handle case statement [nuttx]

Posted by "nuttxpr (via GitHub)" <gi...@apache.org>.
nuttxpr commented on PR #15847:
URL: https://github.com/apache/nuttx/pull/15847#issuecomment-2661022671

   [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
   
   __Fill In The Commit Message:__ This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.
   
   
   
   No, this PR does not fully meet the NuttX requirements.  While it addresses some aspects, it lacks crucial details.
   
   Here's a breakdown of what's missing:
   
   * **Summary:** While it links to the issue, the summary is too brief.  It needs to clearly explain *what* the fix is.  Mentioning "can solve that corner case" is vague.  Explain exactly what the code change *is*.
   * **Impact:**  "New style errors can be reported" is too generic.  Be specific:
       * **User Impact:** Will users see different error messages? Will this affect any existing user code?
       * **Build Impact:**  Will this change the build process at all?
       * **Hardware Impact:**  Unlikely, but explicitly state "NO".
       * **Documentation Impact:** Does the documentation need updating to reflect the new error style?
       * **Security Impact:**  Unlikely, but explicitly state "NO".
       * **Compatibility Impact:** Are there any backward compatibility concerns?  Explicitly state "NO" if none.
   * **Testing:**  The testing description is insufficient.
       * **Build Host(s):**  Provide specifics about your build environment (OS, CPU architecture, compiler version).
       * **Target(s):**  Specify the architecture and board configuration you tested on (e.g., `sim:nsh`, `arm:stm32f4discovery`).
       * **Testing Logs:**  The provided logs only show the *error* message.  Include logs demonstrating the behavior *before* and *after* the fix.  Show that the error is no longer triggered after your change.  Ideally, show the corrected code and the output of nxstyle on the corrected code.
   
   
   In short, the PR needs to be significantly more detailed and explicit in all sections to meet the NuttX requirements.  Provide concrete information, not just general statements.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org