fix(MESHAGEN-006): CU-86akhf8u3 4 review findings across 3 files - #125
flamingo[bot] wants to merge 3 commits into
Conversation
| #define MAC_EVENTS_H_ | ||
|
|
||
| #ifdef __APPLE__ | ||
| #include <Carbon/Carbon.h> |
There was a problem hiding this comment.
🦩 🔴 Unconditional Carbon.h include breaks non-macOS builds
Wrapped the macOS-only body of meshcore/KVM/MacOS/mac_events.h (the #include <Carbon/Carbon.h> line and everything through the MouseAction/KeyAction/KeyActionUnicode declarations) in #ifdef __APPLE__ / #endif /* __APPLE__ */, so on non-Apple builds the header compiles to an empty guarded shell instead of failing on the Carbon.h include. Risk: any non-macOS translation unit that actually relies on these declarations (MouseAction, KeyAction, etc.) being present will now silently lose them rather than fail loudly on the Carbon include; a complete fix would also confirm no non-Apple build path includes this file expecting real declarations.
🤖 Prompt for AI agents
In meshcore/KVM/MacOS/mac_events.h around line 11, review and complete this code-review fix: Unconditional Carbon.h include breaks non-macOS builds.
What the draft fix changed: Wrapped the macOS-only body of meshcore/KVM/MacOS/mac_events.h (the `#include <Carbon/Carbon.h>` line and everything through the `MouseAction`/`KeyAction`/`KeyActionUnicode` declarations) in `#ifdef __APPLE__` / `#endif /* __APPLE__ */`, so on non-Apple builds the header compiles to an empty guarded shell instead of failing on the Carbon.h include. Risk: any non-macOS translation unit that actually relies on these declarations (MouseAction, KeyAction, etc.) being present will now silently lose them rather than fail loudly on the Carbon include; a complete fix would also confirm no non-Apple build path includes this file expecting real declarations.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
| @@ -1,13 +1,14 @@ | |||
| /* | |||
There was a problem hiding this comment.
🦩 🟠 Include guard macro name mismatches actual file (LINUX_EVENTS_H_ in mac_events.h)
Changed the include guard from LINUX_EVENTS_H_ to MAC_EVENTS_H_ (both the #ifndef/#define at the top and the trailing #endif comment), and corrected the header comment from "linux_events.h" to "mac_events.h", exactly as suggested, eliminating the guard collision risk with the Linux counterpart header.
🤖 Prompt for AI agents
In meshcore/KVM/MacOS/mac_events.h around line 1, review and complete this code-review fix: Include guard macro name mismatches actual file (LINUX_EVENTS_H_ in mac_events.h).
What the draft fix changed: Changed the include guard from `LINUX_EVENTS_H_` to `MAC_EVENTS_H_` (both the `#ifndef`/`#define` at the top and the trailing `#endif` comment), and corrected the header comment from "linux_events.h" to "mac_events.h", exactly as suggested, eliminating the guard collision risk with the Linux counterpart header.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -21,10 +21,9 @@ limitations under the License. | |||
| #include "stdafx.h" | |||
There was a problem hiding this comment.
🦩 🔴 WinSock2.h/WS2tcpip.h included unconditionally in WebRTC.cpp
Moved the previously unconditional #include <WinSock2.h> and #include <WS2tcpip.h> at file scope into the existing #if defined(WIN32) guard block (which previously only wrapped crtdbg.h), matching the suggested fix exactly. This prevents these Windows-only headers from being included on POSIX targets while leaving all other code unchanged.
🤖 Prompt for AI agents
In samples/webrtc/C# Sample/WebRTC.cpp around line 21, review and complete this code-review fix: WinSock2.h/WS2tcpip.h included unconditionally in WebRTC.cpp.
What the draft fix changed: Moved the previously unconditional `#include <WinSock2.h>` and `#include <WS2tcpip.h>` at file scope into the existing `#if defined(WIN32)` guard block (which previously only wrapped `crtdbg.h`), matching the suggested fix exactly. This prevents these Windows-only headers from being included on POSIX targets while leaving all other code unchanged.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| #endif | ||
|
|
||
| #if defined(WIN32) | ||
| #if defined(WINSOCK2) |
There was a problem hiding this comment.
🦩 🔴 WINSOCK2/WINSOCK1 headers included without a WIN32 guard
Wrapped the #if defined(WINSOCK2) ... #elif defined(WINSOCK1) ... #endif header inclusion block near the top of the file (previously at line ~22) with an outer #if defined(WIN32) ... #endif guard, matching the style used consistently elsewhere in the file (e.g. the #if defined(WIN32) blocks in ILibAsyncServerSocket_ResumeListeningSink and ILibCreateAsyncServerSocketModuleWithMemoryExMOD). This ensures <winsock2.h>/<ws2tcpip.h> or <winsock.h>/<wininet.h> are only included when building for WIN32, preventing POSIX builds from pulling in Windows-only headers if WINSOCK2/WINSOCK1 macros happen to be defined independently of WIN32.
🤖 Prompt for AI agents
In microstack/ILibAsyncServerSocket.c around line 22, review and complete this code-review fix: WINSOCK2/WINSOCK1 headers included without a WIN32 guard.
What the draft fix changed: Wrapped the `#if defined(WINSOCK2) ... #elif defined(WINSOCK1) ... #endif` header inclusion block near the top of the file (previously at line ~22) with an outer `#if defined(WIN32) ... #endif` guard, matching the style used consistently elsewhere in the file (e.g. the `#if defined(WIN32)` blocks in `ILibAsyncServerSocket_ResumeListeningSink` and `ILibCreateAsyncServerSocketModuleWithMemoryExMOD`). This ensures `<winsock2.h>/<ws2tcpip.h>` or `<winsock.h>/<wininet.h>` are only included when building for WIN32, preventing POSIX builds from pulling in Windows-only headers if WINSOCK2/WINSOCK1 macros happen to be defined independently of WIN32.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
Closes 4 review findings across 3 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
meshcore/KVM/MacOS/mac_events.h:11meshcore/KVM/MacOS/mac_events.h:1samples/webrtc/C# Sample/WebRTC.cpp:21microstack/ILibAsyncServerSocket.c:22What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
bc788a01-3d0c-4679-816f-282037bb5004Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akhf8u3 MeshAgent review findings sweep (13 PRs)