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

[refactoring] Clean code with unified variable type #909

Closed
chenguokai opened this issue Apr 6, 2020 · 7 comments
Closed

[refactoring] Clean code with unified variable type #909

chenguokai opened this issue Apr 6, 2020 · 7 comments

Comments

@chenguokai
Copy link
Collaborator

As far as I can see, many functions use fixed-length variables. It is indeed useful for functions that need to deal with fixed data format. However the abuse of fixed-length variables are causing potential build failures and inessential casts.

For example in src/usb.c:
image

send_recv accept txsize and rxsize as size_t but in the actual context, those arguments have to be casted to int or unsigned int, without a fixed length, and whoever calls it provides arguments of a different type(uint32_t or magic numbers in this case). unintentional cast from 32 bit to 64 bit is applied at the calling process. Also, to prevent a build failure, many explicit casts are required.

I suggest replacing fixed length variables according to their actual usages.
Staying fuzzy to be more portable.

I would like to give it a try if others find it meaningful.

@Nightwalker-87
Copy link
Member

Thx for putting this up on the agenda. As it is a general problem throughout the codebase, I'm not sure at the moment if we should put that into v1.6.1 straight away. @martonmiklos previously requested to do some cleanup related to code style, which I pushed into the v1.6.2 milestone. Maybe this would be a good place for this issue as well, though it will push it out a bit further.

@chenguokai
Copy link
Collaborator Author

Maybe I can do some cleanup locally first and follow up the develop process.😃

@Nightwalker-87
Copy link
Member

Well of course you can, but please ensure to branch off from develop and keep following this branch, instead of master, which is no longer used for frequent changes, but only for releases and hotfixes.

@Nightwalker-87
Copy link
Member

With respect to compatibility between ILP32, LLP64 and LP64 I think it would be a good idea to replace the data type long with int32_t, but if one wants to rule out surprises in the future related to conversion, we may consider to move to fixed width integer types wherever possible...

@Nightwalker-87
Copy link
Member

@chenguokai: I'll leave this to you. You may contribute, if you fancy and have some some time.

@chenguokai
Copy link
Collaborator Author

Sure I will, before the release. Getting busy now.😔

@Nightwalker-87 Nightwalker-87 modified the milestones: v1.6.2, v1.6.3 Dec 25, 2020
@Nightwalker-87 Nightwalker-87 modified the milestones: v1.8.0, v1.7.1 Apr 25, 2021
@Nightwalker-87 Nightwalker-87 changed the title Clean code with unified variable type [refactoring] Clean code with unified variable type Jun 3, 2021
@Nightwalker-87 Nightwalker-87 modified the milestones: v1.7.1, v1.8.0 Aug 15, 2021
@Nightwalker-87 Nightwalker-87 modified the milestones: v1.7.2, Features (Longlist) Aug 27, 2022
@Nightwalker-87 Nightwalker-87 self-assigned this May 7, 2023
@Nightwalker-87 Nightwalker-87 removed this from the Longlist milestone May 7, 2023
@Nightwalker-87 Nightwalker-87 added this to the v1.8.0 milestone May 7, 2023
@Nightwalker-87
Copy link
Member

As there is no common quality level of coding throughout the codebase, the mentioned explicit casting may be excessively used in some parts of the code while others appear to be designed and written quite well.

We are going to use fixed width data types for the first step in order to achieve a unified scheme on this side and most common appearance on different architectures. Further changes related to casting should be individually addressed in a second step (where necessary). Volunteers are welcome to review certain parts and contribute with their ideas and improvements to do so.

@stlink-org stlink-org locked as resolved and limited conversation to collaborators May 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Status: Done
Development

No branches or pull requests

2 participants