【Hackathon 10th Spring No.46】Add Windows C++ compile guards (Part 1/3)#7501
【Hackathon 10th Spring No.46】Add Windows C++ compile guards (Part 1/3)#7501bobby-cloudforge wants to merge 1 commit intoPaddlePaddle:developfrom
Conversation
|
Thanks for your contribution! |
PaddlePaddle-bot
left a comment
There was a problem hiding this comment.
🤖 AI Code Review |
2026-04-20 13:57 CST
📋 Review 摘要
PR 概述:为 custom_ops/gpu_ops/ 下 28 个 C++ 源文件添加 Windows 编译守卫,将 POSIX-only 头文件(fcntl.h、sys/mman.h、unistd.h、sys/ipc.h、sys/msg.h)包裹在 #if !defined(_WIN32) 中,并在使用 POSIX API 的函数体中添加 PD_THROW 运行时保护。
变更范围:custom_ops/gpu_ops/ 及其 speculate_decoding/ 子目录
影响面 Tag:OP
📝 PR 规范检查
PR 标题使用了 【Hackathon 10th Spring No.46】 格式,缺少官方 [Tag]。根据变更目录 custom_ops/gpu_ops/,应添加 [OP] Tag。
标题建议(可直接复制):
[OP] Add Windows C++ compile guards for custom_ops (Part 1/3)
问题
| 级别 | 文件 | 概述 |
|---|---|---|
| 🟡 建议 | custom_ops/gpu_ops/env.h |
新增 <cstdlib> 和 <string> 是修复隐式依赖,非注释/守卫,建议在 PR 描述中补充说明 |
✅ 验证结果
经逐文件审查与代码探索,确认以下关键点均正确:
- 头文件守卫一致性:所有 POSIX-only 头文件(
fcntl.h、sys/mman.h、unistd.h、sys/ipc.h、sys/msg.h)均已正确包裹在#if !defined(_WIN32)中;Windows 上可用的标准头文件(sys/stat.h、sys/types.h、stdio.h)保留在守卫外。 - 函数体守卫完整性:所有使用 POSIX API(
msgsnd/msgrcv/msgget/shm_open/mmap等)的函数体均添加了#if defined(_WIN32) PD_THROW守卫。 - 仅有头文件守卫的
.cu文件(beam_search_softmax.cu、dequant_int8.cu、fused_get_rotary_embedding.cu、fused_hadamard_quant_fp8.cu、stop_generation_multi_ends.cu、share_external_data.cu)已验证其函数体中不直接调用 POSIX API,无需函数体守卫。 - 结构体可移植性:
msgdata、msgdatakv、speculate_msgdata等结构体仅使用long/int等标准 C++ 类型,在 Windows 上可正常编译(虽不会实际用于 IPC 通信)。 custom_ftok.h整文件守卫正确:该文件使用key_t、stat()等 POSIX-only 类型/函数,整文件包裹在#if !defined(_WIN32)中是合理的。所有调用方均在#else(非 Windows)分支内使用custom_ftok()。cuda_multiprocess.h兼容性:该文件(未在本 PR 中修改)已具备完整的 Windows/POSIX 双平台支持,sharedMemoryInfo结构体和sharedMemoryOpen/sharedMemoryClose函数均为跨平台实现。
总体评价
变更机械性强、模式一致,28 个文件的守卫位置均正确。未发现阻塞性问题。作为 Part 1/3,建议后续部分覆盖 set_data_ipc.cu、read_data_ipc.cu 等同样使用 POSIX 共享内存的文件。
|
|
||
| #include <cstdlib> | ||
| #include <string> | ||
|
|
There was a problem hiding this comment.
🟡 建议 env.h 新增了 <cstdlib> 和 <string> 两个头文件,这不是注释或 #ifdef 守卫,而是修复了原有的隐式头文件依赖(std::getenv 需要 <cstdlib>,std::string 需要 <string>)。
建议在 PR 描述的 Modifications 中补充说明此项改动,例如:
Added missing
<cstdlib>and<string>includes toenv.hto fix implicit header dependencies.
Motivation
Add minimal Windows-specific compile guards and clarifying comments to C++ sources to avoid POSIX-only headers being included on Windows.
Modifications
Checklist