Skip to content

include: share covered-switch pragmas [#3595] - #3599

Open
user01010111 wants to merge 3 commits into
networkupstools:masterfrom
user01010111:refactor/covered-switch-pragmas
Open

include: share covered-switch pragmas [#3595]#3599
user01010111 wants to merge 3 commits into
networkupstools:masterfrom
user01010111:refactor/covered-switch-pragmas

Conversation

@user01010111

Copy link
Copy Markdown

Summary

This moves the repeated GCC and Clang warning suppression around defensive
default: clauses into nut-pragmas-covered-switch-default.h and its paired
-end.h header. The headers are intentionally repeat-includable and are
distributed as private build inputs.

The change replaces 41 identical regions across 21 C and C++ translation
units. It does not alter any switch, case, or default body.

Related to #3595. This is one exact warning-family slice and does not close
the broader issue.

Scope

This deliberately leaves the following distinct cases local:

  • include/nutipc.hpp
  • the mixed -Wswitch-enum block in drivers/apcmicrolink.c
  • eight GCC-only regions in drivers/tripplite_usb.c,
    drivers/powerp-bin.c, drivers/adelsystem_cbi.c, and
    drivers/generic_modbus.c

No other warning-suppression family is generalised in this PR.

Validation

  • GCC 16.2.1, hard warning profile with -Werror: full clean build passed.
  • Clang 22.1.8, medium warning profile with -Werror: full clean build
    passed.
  • All 21 changed consumer files compiled in both builds, including the
    available Modbus, Avahi, neon, libusb, nutconf, C, and C++ paths.
  • CppUnit passed with GCC and Clang: 34 tests each.
  • Aggregate test suites passed with GCC and Clang: 9 of 9 each.
  • NIT passed: 26 passed, 0 failed, 0 skipped.
  • GCC and Clang C and C++ probes confirmed repeated inclusion and restoration
    of the warning state after each closing include.
  • make stylecheck, make check-source-nonascii, and whitespace checks
    passed.
  • A bounded distcheck-light run with CHECK_FILES_QUICK_TARGETS= and
    SPELLCHECK_ERROR_FATAL=no passed, including packaging, install,
    uninstall, and distclean checks. The candidate-specific quick checks were
    run separately as listed above.

The Clang hard warning profile stops on 14 warnings treated as errors: ten
existing conversion warnings in common/common.c and four existing global
constructor warnings in common/nutwriter.cpp. Compiling pristine source at
the same revision reproduced the same diagnostics.

An ordinary distcheck-light run is also stopped by existing Aspell findings
in unchanged NEWS.adoc and docs/man/apcmicrolink.txt. Those files match
the base revision byte-for-byte.

Local validation did not exercise Powerman, IPMI, GPIO, Modbus RTU USB,
macOS, or Windows paths. Upstream CI remains responsible for broader platform
coverage.

AI assistance

OpenAI Codex gpt-5.6-sol was used for repository analysis, implementation,
review, drafting, and validation with high reasoning. The human contributor
remains responsible for the contribution.

Move the repeated GCC and Clang diagnostic push/pop sequence for
exhaustive switches with defensive default clauses into intentionally
repeat-includable paired headers.

Replace only the 41 regions that use the same suppression family and
distribute the headers as private build inputs. Leave the mixed
-Wswitch-enum case and compiler-specific regions local.

Refs networkupstools#3595

AI assistance: OpenAI Codex gpt-5.6-sol was used for repository analysis,
implementation, review, drafting and validation with high reasoning. The
human contributor remains responsible for the contribution.

Signed-off-by: user01010111 <lapses.50.booster@icloud.com>
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown

A ZIP file with standard source tarball and another tarball with pre-built docs for commit 2978188 is temporarily available: NUT-tarballs-PR-3599.zip.

@AppVeyorBot

Copy link
Copy Markdown

Build nut 2.8.5.5163-master completed (commit 756e15e844 by @)

@jimklimov jimklimov added refactor/fightwarn PR or issue proposal to improve code maintainability without functional changes, or to fix warnings portability We want NUT to build and run everywhere possible labels Aug 31, 2026
@jimklimov jimklimov added this to the 2.8.6 milestone Aug 31, 2026
…les [networkupstools#3599]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@jimklimov jimklimov added the AI For good or bad, machine tools are upon us. Humans are still the responsible ones. label Aug 31, 2026
Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@AppVeyorBot

Copy link
Copy Markdown

Build nut 2.8.5.5166-master completed (commit 96d7de5c85 by @jimklimov)

@AppVeyorBot

Copy link
Copy Markdown

Build nut 2.8.5.5167-master completed (commit f816b2d532 by @jimklimov)

@biergaizi

biergaizi commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Strongly approve this PR.

These pragmas were added during a project refactor campaign called "fightwarn" with the goal of suppressing all compiler warnings. Unfortunately, because it was rushed, the fixes themselves are often "brute-force" in nature. If the compiler says there's an integer overflow, the fix was to cast all possible integer values instead of fix the location of the overflow. If the compiler says there's a buffer overflow (even to a compiler false-positive), the fix was to add range-checked in all locations where the value is used, rather than fixing the logic.

For example, one commit was introduced to the driver I maintain to suppress an integer overflow warning:

crc16_recv = (uint16_t)((uint16_t)(ident_response_end[0]) << 8) | (uint16_t)(ident_response_end[1]);

I later reverted this change because it was clearly an overfix.

These GCC pragmas are my most disliked commits during the fix. If these changes were subject to review of the maintainer of the respective drivers, I would have rejected them in my driver immediately. Sharing them into a standard header is a choice that I would make from the beginning, if the I don't understand why the copy-and-paste approach was used in the first place.

@jimklimov

Copy link
Copy Markdown
Member

I dislike the screenfulls of these pragmas sprinkled over the code too, and kick myself for not thinking about the include-file approach earlier :)

As for keeping the warnings quiet, they are mostly (hopefully) due to sort-of-overkill in range checks that may be pointless in specific-bitness builds, but may vary between platforms (OTOH, like checking int i <= INT16_MAX which depends on bit-width of a native int, maybe always true, maybe not).

I hope there was not too much brute-forcing per se, although it is possible that attention meandered and some corners got cut as hundreds of very similar cases were addressed by hand or mass replacement patterns...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI For good or bad, machine tools are upon us. Humans are still the responsible ones. portability We want NUT to build and run everywhere possible refactor/fightwarn PR or issue proposal to improve code maintainability without functional changes, or to fix warnings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants