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

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: ***@***.***>

- 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 Use C attribute `[[maybe_unused]]` (introduced in C23) to suppress unused parameter 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.

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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to address compiler warnings #38

Refactor to address compiler warnings #38

bdd commented Apr 23, 2023 •

edited

Loading

schdro Apr 30, 2023

bdd Apr 30, 2023 •

edited

Loading

bdd Apr 30, 2023 •

edited

Loading

robertlipe commented Apr 30, 2023 via email

robertlipe commented Apr 30, 2023 via email

robertlipe commented May 1, 2023 •

edited

Loading

Refactor to address compiler warnings #38

Are you sure you want to change the base?

Refactor to address compiler warnings #38

Conversation

bdd commented Apr 23, 2023 • edited Loading

schdro Apr 30, 2023

Choose a reason for hiding this comment

bdd Apr 30, 2023 • edited Loading

Choose a reason for hiding this comment

bdd Apr 30, 2023 • edited Loading

Choose a reason for hiding this comment

robertlipe commented Apr 30, 2023 via email

robertlipe commented Apr 30, 2023 via email

robertlipe commented May 1, 2023 • edited Loading

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