Skip to content

refactor: replace private access hacks with accessor pattern#383

Merged
zccrs merged 2 commits into
linuxdeepin:masterfrom
zccrs:master
May 26, 2026
Merged

refactor: replace private access hacks with accessor pattern#383
zccrs merged 2 commits into
linuxdeepin:masterfrom
zccrs:master

Conversation

@zccrs
Copy link
Copy Markdown
Member

@zccrs zccrs commented May 20, 2026

Remove all '#define private/protected public' hacks and replace them with a proper C++ template-based private accessor pattern using explicit template instantiation (friend injection trick), mirroring the approach used in linuxdeepin/treeland#875.

Add src/private/dprivateaccessor_p.h with Accessor/AccessorImpl templates and D_DECLARE_PRIVATE_MEMBER, D_DECLARE_PRIVATE_METHOD, D_DECLARE_PRIVATE_CONST_METHOD, D_PRIVATE_MEMBER, D_PRIVATE_CALL macros. All helpers are in global namespace so ADL correctly finds the friend-injected get() function.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR replaces prior #define private/protected public access hacks with a template-based private accessor pattern (friend-injection via explicit instantiation), and migrates a few production call sites to use the new accessors.

Changes:

  • Added src/private/dprivateaccessor_p.h providing macros/templates to access private members/methods without redefining access specifiers.
  • Updated XDG icon proxy engine to call previously-private XdgIconLoader/XdgIconLoaderEngine APIs via accessor tags/macros.
  • Updated Treeland platform window interface to hook QWindow::event without #define protected public, and updated DFontManager to read QFont::d->dpi via accessor.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/util/private/xdgiconproxyengine.cpp Replaces direct private calls/field access with accessor macros (Qt5/Qt6-conditional).
src/util/private/diconproxyengine.cpp Removes #define private public include hack around xdgiconloader_p.h.
src/util/dfontmanager.cpp Replaces QFont::d access hack with accessor-based private member access.
src/private/dprivateaccessor_p.h Introduces the reusable private accessor pattern (templates + macros).
src/plugins/platform/treeland/dtreelandplatformwindowinterface.cpp Replaces protected access hack for QWindow::event with accessor-based member pointer retrieval.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/util/private/xdgiconproxyengine.cpp
Comment thread src/util/dprivateaccessor_p.h
@zccrs zccrs force-pushed the master branch 6 times, most recently from c486e80 to 09bf9f0 Compare May 26, 2026 06:18
zccrs added 2 commits May 26, 2026 18:35
Add CI workflows to verify the project builds correctly on both Arch Linux
(latest) and Deepin crimson. This helps catch any platform-specific
compilation issues introduced by the accessor pattern refactor.
Remove all '#define private/protected public' hacks and replace them with a
proper C++ template-based private accessor pattern using explicit template
instantiation (friend injection trick), mirroring the approach used in
linuxdeepin/treeland#875.

Adds src/util/dprivateaccessor_p.h with Accessor/AccessorImpl templates and
D_DECLARE_PRIVATE_MEMBER, D_PRIVATE_MEMBER macros. All helpers live at global
scope so ADL correctly resolves the friend-injected get() function.
@deepin-ci-robot
Copy link
Copy Markdown
Contributor

deepin pr auto review

这份代码提交主要做了两件事:

  1. 新增 CI 构建流程:为 dtkgui 添加了 Arch Linux 和 Deepin crimson 环境下的 GitHub Actions 自动构建脚本。
  2. 重构私有成员访问方式:移除了极具争议且易引发未定义行为(UB)的 #define private public 宏,转而利用 C++ 显式模板实例化技术实现了一个安全的私有成员/方法访问器 DPrivateAccessor

整体来看,这是一个质量非常高的提交,特别是用标准合规的技术替代了破坏类的访问控制宏,值得称赞。但经过仔细审查,仍有一些语法逻辑、代码性能、安全性和工程质量方面的细节需要改进。

以下是详细的审查意见:


一、 语法与逻辑

1. CI 工作流:步骤顺序导致潜在的构建失败

  • 文件.github/workflows/dtkgui-archlinux-build.yml
  • 问题actions/checkout 被放在了安装依赖的步骤之后。虽然这在大多数情况下能工作,但如果 pacman -Syu(系统更新)更新了 Git,或者由于容器内环境状态问题,晚检出代码可能会遇到意外情况。更重要的是,通常 CI 流程的习惯是优先拉取代码。
  • 建议:将 actions/checkout@v4 移到 Initialize pacman and system update 之前,或者紧跟在其后、安装编译依赖之前。

2. C++ 模板:友元注入在命名空间作用域的潜在风险

  • 文件src/util/dprivateaccessor_p.h
  • 问题:代码注释中已经提到必须在全局命名空间使用这些宏。如果用户在某个命名空间(如 DGUI_BEGIN_NAMESPACE)内使用 D_DECLARE_PRIVATE_MEMBER,由于 ADL(参数依赖查找)的规则和类作用域内友元声明的特殊性,可能会导致 get(Tag) 链接失败或找到错误的函数。
  • 建议
    • 在宏定义处增加编译期静态断言,确保 TagName 不在非全局命名空间(虽然 C++ 缺乏直接反射命名空间的手段,但可以通过 std::is_same_v<decltype(global_tag), TagName> 等变通方法,或严格依赖文档约束)。
    • 强烈建议:在宏展开时加上 :: 限定符,即 friend MemberPtr ::get(TagName) noexcept;,这样可以强制要求 get 函数处于全局命名空间,防止意外的名字冲突。

3. C++ 语法:virtual_hook 返回值处理不一致

  • 文件src/util/private/xdgiconproxyengine.cpp
  • 问题:在 Qt5 中,QIconEngine::virtual_hook 返回类型是 void,原代码是 return engine->virtual_hook(id, data);。重构后变成了:
    #if QT_VERSION < QT_VERSION_CHECK(6, 0, 0)
        return D_PRIVATE_CALL(*engine, XdgIconLoaderEngine_virtualHook_tag{}, id, data);
    #else
        engine->virtual_hook(id, data);
    #endif
    在 Qt5 分支中,D_PRIVATE_CALL 展开后也是一个 void 返回值的调用,return void_expression; 在 C++ 中语法是合法的(通常用于模板转发),但这里与 Qt6 分支的写法不统一,容易让阅读者困惑。
  • 建议:统一写法,去掉 Qt5 分支的 return
    #if QT_VERSION < QT_VERSION_CHECK(6, 0, 0)
        D_PRIVATE_CALL(*engine, XdgIconLoaderEngine_virtualHook_tag{}, id, data);
    #else
        engine->virtual_hook(id, data);
    #endif

二、 代码性能

1. CI 工作流:Deepin 构建缺乏缓存机制

  • 文件.github/workflows/dtkgui-deepin-build.yml
  • 问题:Deepin 的构建流程中,每次都要从源码编译 dtkcommondtklogtreeland-protocolsdtkcore。这些是 dtkgui 的基础依赖,除非大版本升级,否则代码变动较少。每次 CI 都重新 git clonedpkg-buildpackage 会消耗大量 CPU 时间和 CI 资源。
  • 建议:引入 GitHub Actions Cache,将编译生成的 .deb 文件按依赖仓库的 commit hash 进行缓存。如果 hash 未变,直接解压安装缓存的 deb 包,可节省数分钟的构建时间。

2. C++ 运行期:访问器函数的零开销保证

  • 文件src/util/dprivateaccessor_p.h
  • 分析get(Tag)dtk_private_detail::access(Tag) 设计得非常好,它们返回编译期常量(成员指针),且被标记为 inlinenoexcept。现代编译器(GCC/Clang)在开启优化(哪怕是 -O1)时,能够完全内联这些调用,将其优化为直接的内存访问或函数调用,运行期性能损耗为零。无需额外优化。

三、 代码安全

1. CI 安全:APT 仓库使用 [trusted=yes] 且混用 --allow-unauthenticated

  • 文件.github/workflows/dtkgui-deepin-build.yml
  • 问题
    • echo "deb [trusted=yes] ..." 跳过了对仓库的 GPG 签名验证,容易遭受中间人攻击。
    • apt-get install -y --allow-unauthenticated wlr-protocols || true 同样跳过了包的签名验证。
  • 风险:虽然这是在 CI 隔离环境中运行,但如果不慎将此逻辑移植到生产环境或本地开发脚本中,会有供应链攻击风险。
  • 建议:对于 CI 容器,如果确实无法获取官方 GPG key,可以保留 [trusted=yes],但应添加注释说明这是 CI 特定的妥协。对于 --allow-unauthenticated,应尽可能移除,或者确保它仅在完全信任的内网源下使用。

2. C++ 安全:消除了 #define private public 带来的 ODR 违背和 UB 风险

  • 分析:原代码使用宏替换关键字,不仅破坏了 ODR(One Definition Rule),导致包含该头文件的编译单元与不包含的编译单元对同一类的内存布局/访问控制理解不一致,还极易引发难以调试的段错误。新的 DPrivateAccessor 利用 C++ 标准漏洞 [temp.explicit]/12 是一种合法且无 UB的做法,极大提升了代码的安全性和健壮性。

四、 代码质量与工程规范

1. CI 规范:Arch Linux CI 缺乏触发条件限制

  • 文件.github/workflows/dtkgui-archlinux-build.yml
  • 问题:触发条件仅为 on: push:on: pull_request:,这意味着在任何分支上的任何推送(哪怕是修改了 README.md 或文档)都会触发完整的 C++ 编译流程。
  • 建议:增加路径过滤,仅在核心代码变动时触发:
    on:
      push:
        paths:
          - 'src/**'
          - 'CMakeLists.txt'
          - '.github/workflows/dtkgui-archlinux-build.yml'
      pull_request:
        paths:
          - 'src/**'
          - 'CMakeLists.txt'
          - '.github/workflows/dtkgui-archlinux-build.yml'

2. C++ 宏规范:D_DECLARE_PRIVATE_... 宏的展开污染全局命名空间

  • 文件src/util/dprivateaccessor_p.h
  • 问题D_DECLARE_PRIVATE_MEMBERD_DECLARE_PRIVATE_METHOD 宏会在全局命名空间展开 struct TagNametemplate struct DtkGuiPrivateAccessorImpl<...>。这非常容易与其他库的全局符号产生冲突。
  • 建议
    • 强制要求 TagName 遵循严格的命名前缀规范(代码中已经做得很好,如 XdgIconLoader_followColorScheme_tag)。
    • 可以考虑将宏的内部实现细节加上保留标识符前缀(如 __dtk_detail_),避免与用户代码冲突。

3. C++ 代码整洁:dtk_private_detail 命名空间的设计

  • 文件src/util/dprivateaccessor_p.h
  • 分析:引入 dtk_private_detail 命名空间作为 Trampoline 是非常专业的设计。注释中也准确指出了 C++ ADL 的坑(如果类内有同名 get 方法会屏蔽 ADL 查找)。这个 Trampoline 确保了调用上下文的干净,设计值得肯定。

4. 版权声明更新

  • 文件:多个文件
  • 分析:将 SPDX-FileCopyrightText 从 2024 更新为 2024 - 2026。需确认项目是否采用“截止年份”模式。开源社区常见做法是记录首次发表年份或采用 2024-present,不过如果是公司内部规范要求写明截止年份则无妨。

总结

核心 C++ 逻辑的重构(DPrivateAccessor)非常优秀,既解决了痛点又符合标准。主要的改进点集中在 CI 流程的健壮性(步骤顺序、缓存、触发条件)C++ 语法的严谨性(友元命名空间限定、void 返回值统一) 上。建议按照上述意见微调后合入主分支。

@deepin-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743, zccrs

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@zccrs zccrs merged commit 05be780 into linuxdeepin:master May 26, 2026
25 of 26 checks passed
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.

4 participants