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 warnings when building with GCC #38

Merged
merged 5 commits into from
Jan 27, 2025
Merged

Conversation

gnusenpai
Copy link
Contributor

Fixing the warnings is necessary for a successful build when the kernel is built with the CONFIG_WERROR option. This option is default in at least the upstream kernel config.

This PR is marked as a draft because:

  1. It needs testing with clang.
  2. I'm not sure how the nested #ifdef in driver/dbg.h should be formatted.
  3. Removing the static keyword from the fixedpt functions creates warnings in the rust CLI build. While it technically still works, I'm not sure how to fix these. Maybe a different approach should be taken.
    Changing extern to static in the sensitivity function is an alternative fix, but it causes the CLI build to fail until changed back. Building the module with static, reverting it to extern, and then building the CLI works, if that's useful information.

fixes unknown-pragmas warning in GCC
fixes old-style-declaration warning in GCC
Copy link

vercel bot commented Dec 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
maccel ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 27, 2025 0:37am

@MCPO-Spartan-117
Copy link
Contributor

MCPO-Spartan-117 commented Dec 8, 2024

  1. It needs testing with clang.

No errors on clang.

  1. I'm not sure how the nested #ifdef in driver/dbg.h should be formatted.

Should be more modularized but the \s in the functions make that complicated so i don't know how to go about that.

  1. Removing the static keyword from the fixedpt functions creates warnings in the rust CLI build. While it technically still works, I'm not sure how to fix these. Maybe a different approach should be taken.
    Changing extern to static in the sensitivity function is an alternative fix, but it causes the CLI build to fail until changed back. Building the module with static, reverting it to extern, and then building the CLI works, if that's useful information.

@Gnarus-G
Are there any functions that actually need to be static? like need their own version of these functions/results and can't be overwritten later?

   Compiling maccel-cli v0.2.1 (/tmp/git/maccel/cli)
warning: maccel-cli@0.2.1: Compiler version doesn't include clang or GCC: "cc" "--version"
warning: maccel-cli@0.2.1: In file included from src/../../driver/accel.h:5,
warning: maccel-cli@0.2.1:                  from src/libmaccel.c:1:
warning: maccel-cli@0.2.1: src/../../driver/fixedptc.h:216:3: warning: ‘fixedpt_str’ is static but used in inline function ‘fixedpt_cstr’ which is not static
warning: maccel-cli@0.2.1:   216 |   fixedpt_str(A, str, max_dec);
warning: maccel-cli@0.2.1:       |   ^~~~~~~~~~~
warning: maccel-cli@0.2.1: src/../../driver/fixedptc.h:214:15: warning:str’ is static but declared in inline function ‘fixedpt_cstr’ which is not static
warning: maccel-cli@0.2.1:   214 |   static char str[25];
warning: maccel-cli@0.2.1:       |               ^~~

@Gnarus-G
Copy link
Owner

Gnarus-G commented Dec 8, 2024

If you mean just the functions in the fixedpt header file, then no I can't think of a reason they "need" to be.

fixes usage of static fixedpt functions in non-static sensitivity
function warning
@gnusenpai
Copy link
Contributor Author

gnusenpai commented Dec 9, 2024

I figured out a different way to address point number 3 by making a separate wrapper function for the rust side; see the new commit: c045d82. This way fixes the warnings on both the C and rust sides. I'd like feedback on this cause it might be jank.

Should be more modularized but the \s in the functions make that complicated so i don't know how to go about that.

@MCPO-Spartan-117
I tried, but yeah, putting an #ifdef within a #define is either just not possible or really messy. The rest of the file does similar duplication, so it's not really out of place.

driver/dbg.h Outdated Show resolved Hide resolved
driver/dbg.h Outdated Show resolved Hide resolved
@gnusenpai
Copy link
Contributor Author

I'm just gonna open this up, the code styling is up to @Gnarus-G.

@gnusenpai gnusenpai marked this pull request as ready for review December 12, 2024 13:19
driver/accel_rs.h Outdated Show resolved Hide resolved
Gnarus-G and others added 2 commits January 26, 2025 19:36
Co-authored-by: Мастер Чиф <[email protected]>
Co-authored-by: Мастер Чиф <[email protected]>
@Gnarus-G Gnarus-G merged commit 4ff2d8a into Gnarus-G:main Jan 27, 2025
3 checks passed
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.

3 participants