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 -Wextra warnings in real time code #3333

Closed
wants to merge 6 commits into from

Conversation

BsAtHome
Copy link
Contributor

@BsAtHome BsAtHome commented Feb 8, 2025

This PR should fix the real-time part in the -Wextra warnings cleanup.

It turned out that the CFLAGS were not propagated to the RT compile causing the code to be compiled without -Wextra. A whole list of issues turned up after propagating the -Wextra option. The fixed warnings include:

  • unused parameters,
  • signed/unsigned issues,
  • missing field initialization.

Al of these are fixed in this PR when compiling uspace. There may still be problems in the code when compiling with RTAI or Xenomai because some drivers are not compiled in uspace. These issues will have to wait until a local build-environment can be made.

…ameters, signed/unsigned issues and missing initialization.
@BsAtHome BsAtHome marked this pull request as draft February 13, 2025 11:02
@smoe
Copy link
Contributor

smoe commented Feb 18, 2025

Looks good to me. When you cast something like the baudrate to (unsigned), I keep thinking to myself that this should always have been unsigned in the first place. "unset" could just be 0. I just mention this here to see if anyone else wants to comment on this.

@BsAtHome
Copy link
Contributor Author

I pulled this one into draft because it touches componentized files in a way that we may want to discuss.

Many (over 40) .comp files do not use the period parameter in the exported function. You can either forcefully ignore it like I've done here using (void)period, or it can be made to go away by altering halcompile. It would reduce the change-set size to something like 135 files instead of the current 178.

If we introduce option period {yes|no} in the .comp file (default no and parsed by halcompile), then you can control whether or not the (locally) declared function will have period as argument, thus no longer needing to void its presence. There are fewer .comp files that need period than those that do not. A hidden intermediate function would be introduced to capture the export and hide the period parameter. The change would worst case cause a jump because it would be a tail-call. Best case the link-time optimization (LTO) will eliminate the intermediate function completely.

The only disadvantage is that you then need to add the option if you intend to use the period parameter.

That said, changing halcompile is an API change. It may involve user-written code. Either way, they will see a warning (unused parameter period) with this patch or they see an error (period not declared) with the halcompile option way. The question is then: do we walk the "expose users to warning or error" path?

Alternative is to set the option to default yes and add a option period no to the .comp files. The users' code would simply get a warning (unused parameter) and can add the option. They will be no worse off this way. However, all LCNC components that do not use the period parameter will need to have the option added.

@smoe
Copy link
Contributor

smoe commented Feb 18, 2025

With a version LinuxCNC 3 maybe not so far from now, an API change seems possible. I would however opt for the default "option period yes" so no incompatible API change results.

@BsAtHome
Copy link
Contributor Author

This PR has been replaced by #3344 and #3345.

@BsAtHome BsAtHome closed this Feb 21, 2025
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.

2 participants