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

fix(armv8): add missing explicit casts #171

Closed
wants to merge 2 commits into from

Conversation

joaopeixoto13
Copy link
Member

No description provided.

@DavidMCerdeira DavidMCerdeira self-assigned this Sep 13, 2024
@josecm
Copy link
Member

josecm commented Sep 17, 2024

@joaopeixoto13 Can you explain further why these casts are required?

@joaopeixoto13
Copy link
Member Author

@josecm On May 23, this PR introduced additional compiler flags, including -Wconversion, which prompts the compiler to generate warnings for implicit type conversions that could lead to potential issues. This PR includes the necessary explicit casts for the ARMv8 architecture to address these warnings.

@josecm
Copy link
Member

josecm commented Sep 17, 2024

@josecm On May 23, this PR introduced additional compiler flags, including -Wconversion, which prompts the compiler to generate warnings for implicit type conversions that could lead to potential issues. This PR includes the necessary explicit casts for the ARMv8 architecture to address these warnings.

I understand but I don't have any issue in compiling the current main in my machine. I am using aarch64-none-elf-gcc (Arm GNU Toolchain 13.2.rel1 (Build arm-13.7)) 13.2.1 20231009. Are you using a more recent compiler?

@joaopeixoto13
Copy link
Member Author

@josecm On May 23, this PR introduced additional compiler flags, including -Wconversion, which prompts the compiler to generate warnings for implicit type conversions that could lead to potential issues. This PR includes the necessary explicit casts for the ARMv8 architecture to address these warnings.

I understand but I don't have any issue in compiling the current main in my machine. I am using aarch64-none-elf-gcc (Arm GNU Toolchain 13.2.rel1 (Build arm-13.7)) 13.2.1 20231009. Are you using a more recent compiler?

I am using the aarch64-none-elf-gcc (GNU Toolchain for the Arm Architecture 11.2-2022.02 (arm-11.14)) 11.2.1 20220111, which is the same version referenced in Appendix III of the bao-demos guide.

@josecm
Copy link
Member

josecm commented Sep 17, 2024

I am using the aarch64-none-elf-gcc (GNU Toolchain for the Arm Architecture 11.2-2022.02 (arm-11.14)) 11.2.1 20220111, which is the same version referenced in Appendix III of the bao-demos guide.

This is a minimal version and in need of an update. The compiler to be used should be the one in bao-ci's container. If you compile with this version we shouldn't run into any issues with those flags.

@joaopeixoto13
Copy link
Member Author

I am using the aarch64-none-elf-gcc (GNU Toolchain for the Arm Architecture 11.2-2022.02 (arm-11.14)) 11.2.1 20220111, which is the same version referenced in Appendix III of the bao-demos guide.

This is a minimal version and in need of an update. The compiler to be used should be the one in bao-ci's container. If you compile with this version we shouldn't run into any issues with those flags.

Understood, I will update the compiler accordingly. However, would it not be more prudent to include these explicit conversions in the code? Or, given that the compiler handles these automatically, do you believe it is unnecessary?

@josecm
Copy link
Member

josecm commented Sep 17, 2024

Understood, I will update the compiler accordingly. However, would it not be more prudent to include these explicit conversions in the code? Or, given that the compiler handles these automatically, do you believe it is unnecessary?

I think @miguelafsilva5 should answer that.

@miguelafsilva5
Copy link
Member

I believe that if the most recent compiler does not issue any error you should not include those casts. Otherwise we would also have to include other explictit casts throughout the bao code.

Eventually, if those casts are necessary the MISRA analyzer will spot them.

@DavidMCerdeira
Copy link
Member

DavidMCerdeira commented Sep 17, 2024

I'll take @miguelafsilva5 suggestion and close this PR

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.

4 participants