Refactor to address compiler warnings #38

bdd · 2023-04-23T01:15:49Z

Unused function warnings Move function definitions in include/blisp_util.h to lib/blisp_util.c. Defining them as static leads to internal linkage, accessible in that translation unit, hence the warning. Their references are to be handled in other translation units, so they shouldn't be static.
Unused function parameters Define a macro for void casting unused parameters as a portable-across-compilers way to suppress these warnings.
- blisp_chip_xxxxx_get_eflash_loader implementations accept clock type but don't use it.
- drain function only has a body under macOS and FreeBSD with preprocessor predicates.
Enable compiler warnings Now that warnings are address enable them -Wall -Wextra -Wpedantic for the library and the tool targets.

N.B. An equivalent of MSVC should be added to CMakeLists.txt, as these would only work when using GCC or Clang.

Current warnings on master:

[  3%] Building C object CMakeFiles/libblisp_obj.dir/lib/blisp.c.o
/home/bdd/src/github/pine64/blisp/lib/blisp.c: In function ‘drain’:
/home/bdd/src/github/pine64/blisp/lib/blisp.c:16:35: warning: unused parameter ‘port’ [-Wunused-parameter]
   16 | static void drain(struct sp_port* port) {
      |                   ~~~~~~~~~~~~~~~~^~~~
In file included from /home/bdd/src/github/pine64/blisp/lib/blisp.c:3:
/home/bdd/src/github/pine64/blisp/include/blisp_util.h: At top level:
/home/bdd/src/github/pine64/blisp/include/blisp_util.h:83:17: warning: ‘crc32_calculate’ defined but not used [-Wunused-function]
   83 | static uint32_t crc32_calculate(const void *data, size_t data_len)
      |                 ^~~~~~~~~~~~~~~
[  6%] Building C object CMakeFiles/libblisp_obj.dir/lib/chip/blisp_chip_bl60x.c.o
/home/bdd/src/github/pine64/blisp/lib/chip/blisp_chip_bl60x.c: In function ‘blisp_chip_bl60x_get_eflash_loader’:
/home/bdd/src/github/pine64/blisp/lib/chip/blisp_chip_bl60x.c:7:52: warning: unused parameter ‘clk_type’ [-Wunused-parameter]
    7 | int64_t blisp_chip_bl60x_get_eflash_loader(uint8_t clk_type, uint8_t** firmware_buf_ptr)
      |                                            ~~~~~~~~^~~~~~~~
[  9%] Building C object CMakeFiles/libblisp_obj.dir/lib/chip/blisp_chip_bl70x.c.o
/home/bdd/src/github/pine64/blisp/lib/chip/blisp_chip_bl70x.c: In function ‘blisp_chip_bl70x_get_eflash_loader’:
/home/bdd/src/github/pine64/blisp/lib/chip/blisp_chip_bl70x.c:7:52: warning: unused parameter ‘clk_type’ [-Wunused-parameter]
    7 | int64_t blisp_chip_bl70x_get_eflash_loader(uint8_t clk_type, uint8_t** firmware_buf_ptr)
      |                                            ~~~~~~~~^~~~~~~~
[ 12%] Building C object CMakeFiles/libblisp_obj.dir/lib/blisp_easy.c.o
In file included from /home/bdd/src/github/pine64/blisp/lib/blisp_easy.c:4:
/home/bdd/src/github/pine64/blisp/include/blisp_util.h:24:13: warning: ‘sleep_ms’ defined but not used [-Wunused-function]
   24 | static void sleep_ms(int milliseconds) {

schdro · 2023-04-30T09:25:46Z

include/blisp.h

 void blisp_device_close(struct blisp_device* device);

-#endif
+#define UNUSED(x) (void)(x)


Sidenote:
Root CMakeLists.txt requests C standard 23 via CMAKE_C_STANDARD. C23 introduced official maybe_unused attribute. Using latter might avoid clumsiness of ancient C helper construct and ease portability ("portable-across-compilers").
Ultimately, also matter of style.

Thanks. I had no idea this made it to C standard, assumed it was C++ only. Didn't notice this project asked for C23, a pleasant surprise. Updated to use the attribute instead.

Well, MSVC's C23 support is...what's the word I'm looking for...sparse. [[maybe_unused]] is not implemented, yet.

From: https://en.cppreference.com/w/c/compiler_support/23

Compilation failure from GHA https://github.com/bdd/blisp/actions/runs/4845978847/jobs/8635225026

robertlipe · 2023-04-30T09:59:59Z

If C23 is already required (really?!?!) and you can propose a patch that passes presubmit on all the platforms that matter (in the real world, standards conformance is a spectrum, not an enum) I don't see the harm in using [[maybe_unused]] and friends. The problem we (and every other project in the real world) have is that we have to work on compilers that are actually provided and work on real platforms. That may be GCC 13 on FreeBSD but GCC 11 on Linux and LLVM 12 on MacOS (all numbers made up). To me, the standards enforced by presumit are the actual floor of requirements, so if you propose a change that passes presubmits and uses C23 features, I'm likely to approve it. To the original patch, it's worth mentioning that -Wall is a moving target. -Wall on FreeBSD may have different warnings that -Wall on Linux. IME it's best to express the warnings you actually care about explicitly if you don't have a large number of active developers on a large number of platforms (and this project doesn't...)

…

On Sun, Apr 30, 2023 at 4:25 AM schrob ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/blisp.h <#38 (comment)>: > \ No newline at end of file +#define UNUSED(x) (void)(x) Sidenote: Root CMakeLists.txt requests C standard 23 via CMAKE_C_STANDARD. C23 introduced official maybe_unused attribute. Using latter might avoid clumsiness of ancient C helper construct and ease portability ("portable-across-compilers"). Ultimately, also matter of style. — Reply to this email directly, view it on GitHub <#38 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD36O7JVLOCYDF67UEBLXDYV2NANCNFSM6AAAAAAXIFWAOY> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

robertlipe · 2023-04-30T22:10:24Z

I don't represent official stance on this project, but speaking for my own projects... If I say that my project requires X and some compiler doesn't support X and X isn't represented in the presubmit testing, then that's just another of the infinite combinations of life that doesn't work and not a reason to use X. I'm also realistic and don't wait for every compiler to implement the entirety of X because none of my projects use the WHOLE language/libraries. In this case, an iddef to dumb code down for MSVC would be trivial, but it's not a reason, IMO, to hold the rest of code back. Even if [[foo]] isn't implemented, it was the clear intent of the ISO group to ignore or warn against unknown foo decorators, so I'd hope this wouldn't be a build error anyway... But MSVCs open contempt of C standards is decades long and hard to overestimate so I wouldn't guess what it does. MSVC has held the industry back for 20+ years. This would hardly be the first project to just not care. It's an awful complier with a good debugger. I'll try to review the patch in full later tonight.

…

On Sun, Apr 30, 2023, 3:06 PM Berk D. Demir ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/blisp.h <#38 (comment)>: > \ No newline at end of file +#define UNUSED(x) (void)(x) Well, MSVC's C23 support is...what's the word I'm looking for...sparse. [[maybe_unused]] is not implemented, yet. [image: image] <https://user-images.githubusercontent.com/11135/235373879-6015224b-7fbd-4715-b222-f4b199d0c7e4.png> From: https://en.cppreference.com/w/c/compiler_support/23 — Reply to this email directly, view it on GitHub <#38 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD37QYI6XM7U532G36VTXD3A2ZANCNFSM6AAAAAAXIFWAOY> . You are receiving this because you commented.Message ID: ***@***.***>

robertlipe · 2023-05-01T19:03:53Z

Oh. I see now there IS a failing presubmit on this. (Why doesn't the GH mailer make that more clear? Ugh!) That's both fortunate (it caught the issue) and unfortunate (it claims to require support for C23, but supports a compiiler that doesn't really support C23.). As [[maybe_unused]] was part of C++17, it would be pretty hamfisted if they special-cased in the C front-end.

Can you please upgrade the requirement in workspaces/whatever.yaml to try to use the latest MSVC as it looks like it might be using a 2019 version
-- The C compiler identification is MSVC 19.34.31944.0
so maybe getting to the latest from MSVC in the presubmit build image will help.

Barring that, it looks like the solution may be to try something approximately like:

#if defined(_MSC_VER) 
#  define MAYBE_UNUSED(x) __pragma(warning(suppress:4100))
#else
#  define MAYBE_UNUSED(x) [[maybe_unused]]x
#endif

(Code is free-handed. Expect problems.)

Please try to trigger on _MSC_VER since we're tip-toeing around broken MSVC and not WIN32; people use Clang, GCC, or other working compilers on Windows.

SO has many hint, but many are based on C++ (wheere this has been required since C++17) or otherwise before C23. There are a few answers that consider it, though, such as https://stackoverflow.com/a/75965534

Sorry, but It's simply annoying that there are so many different ways to spell this, though most compilers have had some form of this for 10+ years in their C/C++ front-ends. This is why the C/C++ groups (finally) decided to unify some of this stuff very recently.

Ralim

These changes seem sane to me. Your probably going to run into argtable3 being out of date at the least like I did. That should fix some of the build errors too

bdd · 2025-07-10T05:49:05Z

These changes seem sane to me. Your probably going to run into argtable3 being out of date at the least like I did. That should fix some of the build errors too

Looks like there's at least one more thing to fix: strformat(3) correctness when printing size_t as hex. https://github.com/pine64/blisp/actions/runs/16187001395/job/45694445341?pr=38#step:3:1069

I will add that.

Oddly this warning is only popping up in armv7 build, but not in aarch64, or riscv64 build actions, or locally on x86_64-linux using GCC 14.3.0

Ralim · 2025-07-17T08:55:46Z

@bdd if you rebase hopefully some errors go away now

Move function definitions in include/blisp_util.h to lib/blisp_util.c. Defining them as static leads to internal linkage, accessible in that translation unit, hence the warning. Their references are to be handled in other translation units, so they shouldn't be static.

We could use C23's `[[maybe_unused]]` but MSVC doesn't support it. So we void cast them with an accompanying "unused" comment.

blisp.h and common.h declares signatures of functions that return blisp_return_t as int32_t.

sp_* functions from libserialport return `enum sp_return`, not `blisp_return_t`.

Improve blisp_flash_firmware error handling: - Address never checked return of `parse_firmware_file` - Ensure the function always returns the correct blisp_return_t value - Make error checking syntactic dance consistent

`z` is the length field for `size_t`.

Ralim · 2025-07-18T03:06:38Z

CMakeLists.txt

+        lib/chip/blisp_chip_bl70x.c)

 target_include_directories(libblisp_obj PRIVATE ${CMAKE_SOURCE_DIR}/include/)
+if (NOT CMAKE_C_COMPILER_ID MATCHES "MSVC")


I'm guessing this isn't working with the build failures?

it is working. there's a missing one. i'm working on sending something that'll address the build issue but MSVC gives a useful warning neither gcc nor clang gave.

blisp_send_command receives a uint16_t type payload_size, but in various places what is passed to it is uint32_t. I was reading the call sites and went down the rabbit hole of Bouffalo Lab docs.

Let me update this real quick, retaining the warnings. They can be worked on later.

Ahh makes sense. No pressure/rush was trying to help 😓

Everything is green right now, but I think MSVC's warning highlights a possibly unintended but currently benign type down conversion issue.

From: https://github.com/pine64/blisp/actions/runs/16361436024/job/46229915025?pr=38#step:4:35

[...]\lib\blisp.c(196,59): warning C4244: 'initializing': conversion from 'float' to 'uint32_t', possible loss of data [...]\lib\blisp.c(286,56): warning C4244: 'function': conversion from 'uint32_t' to 'uint16_t', possible loss of data [...]\lib\blisp.c(385,61): warning C4244: 'function': conversion from 'uint32_t' to 'uint16_t', possible loss of data

So, for the first one, float to uint32_t, what is missing is an explicit conversion. I will add it as a separate commit. The other two, are about the uint16_t payload_size parameter of blisp_send_command.

OK. I'm done :)
If you think it's OK to merge with the two warnings only MSVC raises, I'm OK too.

I can work on these warnings in the near future. I promise it won't take me 2 years to come back to it (check the age of this PR 😅).

I'm also to blame for the age, no one was really monitoring PR's well and I forgot about it 😓

I think its good to go indeeed

schdro reviewed Apr 30, 2023

View reviewed changes

bdd force-pushed the compiler-warnings branch from 66b6f30 to e959756 Compare April 30, 2023 17:29

bdd force-pushed the compiler-warnings branch from e959756 to c0e00e9 Compare July 10, 2025 05:29

Ralim reviewed Jul 10, 2025

View reviewed changes

bdd force-pushed the compiler-warnings branch from c0e00e9 to a715281 Compare July 10, 2025 05:34

bdd force-pushed the compiler-warnings branch from a715281 to 94f59d5 Compare July 10, 2025 05:59

Ralim mentioned this pull request Jul 17, 2025

BL808 Support #57

Merged

bdd added 6 commits July 17, 2025 14:59

Fix compiler warnings: Unused function params

77df78c

We could use C23's `[[maybe_unused]]` but MSVC doesn't support it. So we void cast them with an accompanying "unused" comment.

Fix compiler warnings: enum-int-mismatch

847be6c

blisp.h and common.h declares signatures of functions that return blisp_return_t as int32_t.

Fix compiler warnings: enum-conversion

77410ad

sp_* functions from libserialport return `enum sp_return`, not `blisp_return_t`.

Fix compiler warnings: blisp_flash_firmware

20270e5

Improve blisp_flash_firmware error handling: - Address never checked return of `parse_firmware_file` - Ensure the function always returns the correct blisp_return_t value - Make error checking syntactic dance consistent

Fix compiler warnings: format string

cb89bdb

`z` is the length field for `size_t`.

bdd force-pushed the compiler-warnings branch 2 times, most recently from 679e9a5 to 9cbb817 Compare July 17, 2025 22:17

Ralim approved these changes Jul 17, 2025

View reviewed changes

Ralim reviewed Jul 18, 2025

View reviewed changes

Enable all compiler warnings

d60e004

bdd force-pushed the compiler-warnings branch from 9cbb817 to d60e004 Compare July 18, 2025 03:26

Fix compiler warnings: Explicit cast where needed

ee10c51

Ralim merged commit 5580eec into pine64:master Jul 18, 2025
9 checks passed

BrewTestBot mentioned this pull request Aug 1, 2025

blisp 0.0.5 Homebrew/homebrew-core#231985

Merged

Refactor to address compiler warnings #38

Refactor to address compiler warnings #38

Uh oh!

Conversation

bdd commented Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdd Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdd Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertlipe commented Apr 30, 2023 via email

Uh oh!

robertlipe commented Apr 30, 2023 via email

Uh oh!

robertlipe commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ralim left a comment

Choose a reason for hiding this comment

Uh oh!

bdd commented Jul 10, 2025

Uh oh!

Ralim commented Jul 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bdd commented Apr 23, 2023 •

edited

Loading

bdd Apr 30, 2023 •

edited

Loading

bdd Apr 30, 2023 •

edited

Loading

robertlipe commented May 1, 2023 •

edited

Loading