Conversation
|
@codex review |
|
Some findings that I accidentally produced:
|
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive Windows build support to picoquic, enabling compilation using MSVC, MinGW, and other Windows toolchains. It implements the missing gettimeofday() function for Windows through wintimeofday() and ensures proper header inclusion for Windows networking APIs.
Changes:
- Implements
wintimeofday()function to provide Windows-compatiblegettimeofday()functionality - Updates headers to auto-detect Windows platform and define
_WINDOWSmacro consistently - Integrates Windows compatibility files into build systems (CMake and Visual Studio projects)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| picoquic/wincompat.c | New implementation of wintimeofday() function that converts Windows FILETIME to Unix timeval |
| picoquic/wincompat.h | Enhanced with automatic _WINDOWS detection, ws2tcpip.h inclusion, and gettimeofday macro mapping |
| picoquic/picoquic.h | Added early _WINDOWS detection for correct header ordering |
| picoquic/picosocks.h | Added early _WINDOWS detection for consistency |
| picoquic/picoquic_packet_loop.h | Added Windows-specific ssize_t typedef and Windows-compatible function signature for picoquic_packet_loop_v3 |
| CMakeLists.txt | Added wincompat.c to build on Windows and provides compatibility files to fetched picotls dependency |
| picoquic/picoquic.vcxproj | Added wincompat.c to Visual Studio project sources |
| picoquic/picoquic.vcxproj.filters | Added wincompat.c to Source Files filter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
The changes still seem questionable to me. For example, "Missing |
… _WINDOWS argument
|
@Mygod Thank you for the review. I've researched the issues raised:
I made some additional changes:
|
|
It exists in picotls. Please refer to my comment in the other PR. |
|
@Mygod Thanks for the pointer. I’ve switched to picotls’ wintimeofday.c and dropped the duplicate in picoquic. All tests pass. |
Adds comprehensive Windows build support to picoquic, enabling compilation on Windows using MSVC, MinGW, and other Windows toolchains.
Changes
Core Windows Compatibility
picoquic/wincompat.c: New file implementingwintimeofday()function, providing a Windows-compatible implementation ofgettimeofday()for picotls and picoquicpicoquic/wincompat.h: Enhanced to:_WINDOWSmacro when Windows is detected via standard macros (_WIN32,__WIN32__,__MINGW32__, etc.)<ws2tcpip.h>for IPv6 socket structures (sockaddr_in6, etc.)gettimeofdaymacro mapping towintimeofdayBuild System Updates
CMakeLists.txt:wincompat.ctopicoquic-corelibrary sources when building on WindowsPICOQUIC_FETCH_PTLS=ON, automatically provides Windows compatibility files to picotls:wincompat.hto picotls include directorywincompat.cto picotls lib directorywincompat.ctopicotls-coretargetpicoquic/picoquic.vcxproj: Addedwincompat.cto Visual Studio projectpicoquic/picoquic.vcxproj.filters: Addedwincompat.cto Source Files filterHeader File Updates
picoquic/picoquic.h: Added early_WINDOWSdetection to ensure correct header inclusion orderpicoquic/picosocks.h: Added early_WINDOWSdetectionpicoquic/picoquic_packet_loop.h: Addedssize_ttype definition for Windows compatibilityWhy This Is Needed
Missing
gettimeofday(): Windows doesn't providegettimeofday(), which is required by picotls. Thewintimeofday()implementation converts WindowsFILETIMEto Unixtimeval.IPv6 Support: Windows requires
<ws2tcpip.h>for IPv6 socket structures, which was missing from the compatibility header.Build Integration: The existing
wincompat.handwincompat.cfiles weren't being compiled into the library, causing link errors on Windows.Backward Compatibility
#ifdef _WINDOWSorif(WIN32))#ifdef _WINDOWScontinues to work without modification