Skip to content

Fix latent bugs and reduce compiler warnings by 33% - #493

Closed
dieseld23 wants to merge 2 commits into
temcocontrols:masterfrom
dieseld23:fix/compiler-warnings-and-latent-bugs
Closed

dieseld23 wants to merge 2 commits into
temcocontrols:masterfrom
dieseld23:fix/compiler-warnings-and-latent-bugs

Conversation

@dieseld23

Copy link
Copy Markdown

Cleans up the compiler warning noise in the build and, in the process, fixes four genuine defects that were hiding in it.

Results

Clean Rebuild of T3000 - VS2019.sln (x86 / Release), 0 errors:

Before After
Total warnings 799 533 (−33%)
C4091 typedef ignored 253 4
C4715 not all paths return 25 12
C4717 infinite recursion 1 0
C4716 missing return value 1 0
C4172 dangling pointer 2 0
C4702 unreachable code 0 0
C4094 untagged struct 0 0

The last two rows are the regression check: no new warnings were introduced, and C4702 remaining at 0 confirms that none of the inserted return statements landed in a function that already returned on every path.

What changed

Two commits, deliberately separated so the risky and the risk-free parts can be reviewed differently.

1. Latent bug fixes (13 files) — can affect runtime behaviour:

  • CWorkspaceBar::OnMouseMove called itself rather than the base class. Any mouse move over that pane was a guaranteed stack overflow.
  • set_offline_mode is declared bool and dllexported but never returned, so callers read a junk register. All five callers ignore the result, so the fix preserves behaviour and keeps the exported signature.
  • intervaltotextfull and intervaltotext_old both returned a pointer to a 12-byte stack local. Every caller passes an output buffer and ignores the return value.
  • 13 missing return statements on paths that fell off the end of a non-void function.

2. typedef cleanup (6 files) — provably no-op: 17 declarations like typedef struct Foo { ... }; have no alias after the closing brace, so the compiler was already discarding the keyword.

Notes for the reviewer

Return values were chosen conservatively. Each follows a convention already present in the same function or API — mudbus_read_one returns -2 on retry exhaustion to match its sibling loop; S_OK for the WebView2 completion handler; 0 for the MFC ON_MESSAGE handler.

Ten C4715 sites were deliberately left unfixed. Where the correct return value could not be determined from the code, guessing would convert today's non-determinism into silent, deterministic incorrect behaviour — strictly worse. These need someone with protocol knowledge:

  • Post_Refresh_Message is the one to look at. Its MODBUS branch has its entire body commented out including the return TRUE/FALSE. That reads like an unfinished edit and may be a real functional bug rather than just a warning.
  • WritePrivateBacnetToModbusData (×2, T3000 and ISP copies) — is the timeout path 0 or -1?
  • ShutDownMstpGlobal has a bare return; in an int function.
  • Plus isbacnet_point, check_bacnet_data, Load_Product_Value_Cache, GetPriavteDataByPanelBlocking, ReadDataAgain, WritePrivateBacnetToModbusCharData.

Two further issues found but not touched here:

  • dlmstp.c:744,749 pass narrow string literals to CreateSemaphore in a Unicode build (C4133 ×2), so both semaphore names are garbage. A _T() wrap away, but it belongs in its own change.
  • A third copy of intervaltotext_old with the identical dangling-pointer bug lives in T3000ProgramMoudle/dllmain.cpp:1630. Left alone because that project is not referenced by this solution and therefore isn't built.

Only declarations closing with a bare }; were touched in commit 2. Ones that do define an alias (e.g. } URLStruct;) were left alone — stripping typedef there would turn a type alias into a variable definition.

Unrelated, but worth knowing: upstream CI has been failing since ~2026-06-10, when GitHub's windows-latest moved to Visual Studio 2026. The runner lacks C++ MFC for v143 and the .NET Framework 4.5.2 targeting pack, producing MSB8041/MSB3644. That predates this branch and is not affected by it.

🤖 Generated with Claude Code

dieseld23 and others added 2 commits September 18, 2026 18:09
Four of these are genuine defects, not style issues:

- CWorkspaceBar::OnMouseMove called itself instead of the base class
  (C4717), guaranteeing a stack overflow whenever the handler ran.
  Now calls CDockablePane::OnMouseMove.

- set_offline_mode is declared bool and dllexport'ed but never returned
  (C4716), so callers read whatever happened to be in the return
  register. All five callers ignore the result, so returning the stored
  value preserves behaviour and keeps the exported signature intact.

- intervaltotextfull and intervaltotext_old both returned a pointer to a
  12-byte stack local (C4172). Every caller passes an output buffer and
  ignores the return value, so returning that buffer is equivalent and
  removes the dangling pointer.

The remaining 13 changes add missing return statements on control paths
that fell off the end of a non-void function (C4715). Each was checked
individually; values follow the convention already used elsewhere in the
same function (e.g. mudbus_read_one returns -2 on retry exhaustion,
matching its sibling loop) or the surrounding API (S_OK for the WebView2
completion handler, 0 for the MFC ON_MESSAGE handler).

Sites where the correct return value could not be determined from the
code were deliberately left alone: guessing wrong would turn today's
non-determinism into silent, deterministic incorrect behaviour.

Verified with a clean Rebuild of "T3000 - VS2019.sln" (x86/Release):
0 errors, and C4702 stays at 0, confirming no inserted return landed in
a function that already returned on all paths.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seventeen declarations of the form

    typedef struct Foo { ... };
    typedef enum { ... };

have no alias after the closing brace, so the typedef does nothing and
MSVC discards it with C4091. These were by far the loudest warning in
the build: 253 of 799 warning lines came from a handful of headers,
repeated once per translation unit that included them.

Dropping the keyword is a no-op by construction -- the compiler was
already treating these as plain struct/enum definitions. Existing code
referring to the tag names is unaffected.

Only declarations whose body closes with a bare "};" were touched.
Declarations that do define an alias (e.g. "} URLStruct;") were left
alone; stripping typedef there would convert a type alias into a
variable definition.

C4091 drops from 253 to 4; the remaining 4 originate in Windows SDK
headers and cannot be fixed here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dieseld23

Copy link
Copy Markdown
Author

Opened against the wrong repository by mistake -- this was intended for the dieseld23 fork, not upstream. Closing. Apologies for the noise.

@dieseld23 dieseld23 closed this Sep 18, 2026
@dieseld23
dieseld23 deleted the fix/compiler-warnings-and-latent-bugs branch September 23, 2026 01:24
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.

1 participant