Skip to content

toolchains: Compiler versioning adds --print-memory-usage#14620

Merged
xiaoxiang781216 merged 1 commit into
apache:masterfrom
W-M-R:toolchains
Nov 5, 2024
Merged

toolchains: Compiler versioning adds --print-memory-usage#14620
xiaoxiang781216 merged 1 commit into
apache:masterfrom
W-M-R:toolchains

Conversation

@W-M-R

@W-M-R W-M-R commented Nov 4, 2024

Copy link
Copy Markdown
Contributor

Summary

  1. The "-print-memory-usage" parameter introduced in GNU Link version 2.26

Impact

unnecessary

Testing

unnecessary

@github-actions github-actions Bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small labels Nov 4, 2024
@nuttxpr

nuttxpr commented Nov 4, 2024

Copy link
Copy Markdown

[Experimental Bot, please feedback here]

This PR appears not to fully meet the NuttX requirements, specifically in the "Impact" and "Testing" sections. While the summary provides a reasonable explanation of why the change is necessary, it lacks crucial details:

  • Missing Details in Summary: The summary explains the problem (lack of --print-memory-usage support) but doesn't explain what the PR changes to fix it (the how). What is the dynamic detection mechanism? How is it implemented? What code is modified?

  • Insufficient Impact Assessment: Simply stating "unnecessary" for impact is not acceptable. Even if the user-facing functionality doesn't change, there are potential impacts. For instance:

    • Impact on build: Does the dynamic detection add any build steps or dependencies? Does it change the build output in any way (even if just log messages)?
    • Impact on compatibility: Does this change affect compatibility with any existing tools or scripts that rely on the build process? Does the dynamic detection mechanism work across all supported build environments?
    • Impact on hardware: Even if the intention is no hardware impact, explicitly confirming this is important. Does the change introduce any overhead or code size increases that could affect resource-constrained targets?
  • Missing Testing Information: "Unnecessary" is also insufficient for the Testing section. Even if the change seems minor, testing is crucial. At a minimum, you should demonstrate:

    • Build Host: Specify the build host used to validate the change.
    • Target(s): Specify the target platform(s) where the change was tested.
    • Test Procedure: Describe the steps taken to verify the dynamic detection mechanism. Did you test with a toolchain that does support --print-memory-usage and one that doesn't?
    • Test Results: Show some output, even if just a build log snippet demonstrating the dynamic detection working as expected.

In short, the PR description needs to be more comprehensive and provide specific details about the change, its impact, and the testing performed. Even small changes require thorough documentation to ensure code quality and maintainability.

Comment thread arch/risc-v/src/cmake/Toolchain.cmake Outdated
1. The "-print-memory-usage" parameter introduced in GNU Link version 2.26

Signed-off-by: wangmingrong1 <wangmingrong1@xiaomi.com>
@W-M-R W-M-R changed the title toolchain: Print memory usage using dynamic detection to see if the parameter is supported toolchains: Compiler versioning adds --print-memory-usage Nov 5, 2024
@github-actions github-actions Bot removed the Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture label Nov 5, 2024
@xiaoxiang781216
xiaoxiang781216 merged commit 6381685 into apache:master Nov 5, 2024
@W-M-R
W-M-R deleted the toolchains branch November 11, 2024 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants