[PR] arch/x86_64/include/intel64/irq.h: align definitions [nuttx]

["szafonimateusz-mi (via GitHub)" <gi...@apache.org>]

szafonimateusz-mi opened a new pull request, #15408:
URL: https://github.com/apache/nuttx/pull/15408

   ## Summary
   align all definitions in intel64/irq.h
   
   ## Impact
   
   cosmetic change
   
   ## Testing
   
   local build
   
   
   


-- 
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] arch/x86_64/include/intel64/irq.h: align definitions [nuttx]

["xiaoxiang781216 (via GitHub)" <gi...@apache.org>]

xiaoxiang781216 merged PR #15408:
URL: https://github.com/apache/nuttx/pull/15408


-- 
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] arch/x86_64/include/intel64/irq.h: align definitions [nuttx]

["nuttxpr (via GitHub)" <gi...@apache.org>]

nuttxpr commented on PR #15408:
URL: https://github.com/apache/nuttx/pull/15408#issuecomment-2567863769

   [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
   
   
   
   No.  The PR description lacks critical information required by the NuttX guidelines. While it addresses the summary with a brief description, it falls short in other areas:
   
   * **Insufficient Summary Detail:** "align all definitions in intel64/irq.h" is vague. *Why* was alignment necessary? Was there a bug?  What specific definitions were changed?  How were they aligned (e.g., using compiler attributes, manual padding)?
   
   * **Missing Impact Assessment:**  While "cosmetic change" is a start, it's not enough.  Even cosmetic changes can have unintended consequences.  Explicitly state "NO" for each impact category or explain if there's any nuanced impact. For example, does the change affect binary size?
   
   * **Inadequate Testing Information:** "local build" is insufficient.  Specify the *host* operating system, compiler, and version.  Crucially, specify the *target* architecture and board configuration. Provide *actual* testing logs (even if they seem trivial for a cosmetic change), showing the relevant behavior before and after the change.  Demonstrate that the change has the intended effect and doesn't introduce regressions.
   
   
   **Example of how to improve the PR description:**
   
   ## Summary
   
   This change aligns all structure definitions within `intel64/irq.h` to improve code readability and potentially improve performance by ensuring natural alignment for data members.  This was done by adding `__attribute__((packed))` where appropriate. This addresses inconsistencies in the existing code where some structures were implicitly aligned and others were not.
   
   ## Impact
   
   * Is new feature added? NO
   * Is existing feature changed? NO
   * Impact on user: NO
   * Impact on build: NO (Binary size may change slightly but is not expected to be significant).
   * Impact on hardware: NO
   * Impact on documentation: NO
   * Impact on security: NO
   * Impact on compatibility: NO
   * Anything else to consider?  Potentially minor changes in binary size.
   
   ## Testing
   
   I confirm that changes are verified on local setup and works as intended:
   * Build Host(s): Linux (Ubuntu 22.04), x86_64, GCC 11.3.0
   * Target(s): QEMU-x86_64:nsh
   
   
   Testing logs before change:
   ```
   NuttShell (NSH)
   nsh> size nuttx
      text    data     bss     dec     hex filename
     12345   6789    1011   20145    4eb9 nuttx  <-- Example size
   nsh>
   ```
   
   Testing logs after change:
   ```
   NuttShell (NSH)
   nsh> size nuttx
      text    data     bss     dec     hex filename
     12345   6788    1011   20144    4eb8 nuttx  <-- Example size (potentially slight change)
   nsh>
   ```
   
   
   This improved example demonstrates more thoroughness and provides the reviewer with much more context and confidence in the change. Remember to adapt it to your specific situation and provide relevant test logs that showcase the impact (or lack thereof) of your changes.
   


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