Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

internal: polish native link flags #428

Merged
merged 2 commits into from
Oct 29, 2024
Merged

internal: polish native link flags #428

merged 2 commits into from
Oct 29, 2024

Conversation

Young-Flash
Copy link
Collaborator

No description provided.

Copy link

peter-jerry-ye-code-review bot commented Oct 29, 2024

Observations and Suggestions

  1. MSVC Setup in CI Workflow:

    • Problem: The Setup MSVC step is repeated in both the msys2 and choco sections of the CI workflow.
    • Suggestion: Consider consolidating the Setup MSVC step to avoid redundancy. This could be moved to a common section before the build steps to ensure it runs regardless of the package manager used.
  2. Compiler Check in set_native_backend_link_flags:

    • Problem: The compiler check and error message assume the presence of cc for Unix-like systems but do not handle Windows specifics well.
    • Suggestion: Use a more generic approach to check for the compiler based on the operating system. For Windows, check for cl (the Microsoft C++ compiler) instead of cc. This can be achieved by defining the compiler name conditionally based on the platform (as shown in the diff).
  3. NativeLinkConfig Default Implementation:

    • Problem: The NativeLinkConfig struct lacks a default implementation, which could lead to repetitive and error-prone initialization code.
    • Suggestion: Add a Default implementation to NativeLinkConfig to simplify the initialization process and reduce redundancy. This is shown in the diff with the #[derive(Default)] attribute added to the struct.

Additional Notes

  • Redundancy in CI Workflow: The repeated Setup MSVC step suggests a need for a more streamlined approach to setting up the environment across different OS configurations. This could improve maintainability and reduce the risk of inconsistencies.
  • Conditional Compilation: The use of #[cfg(unix)] and #[cfg(windows)] to handle platform-specific logic is a good practice. Ensure that all relevant sections of the code are appropriately wrapped to avoid runtime errors on different platforms.
  • Configuration Structs: The addition of Default to NativeLinkConfig not only simplifies the code but also ensures that instances of this struct can be created with reasonable default values, which is especially useful in complex systems with many configuration options.

@Young-Flash Young-Flash merged commit 3337443 into main Oct 29, 2024
14 checks passed
@Young-Flash Young-Flash deleted the native_flags branch October 29, 2024 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant