refactor: replace private access hacks with accessor pattern#625
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zccrs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR removes preprocessor-based private/protected access hacks and replaces them with a template-based private accessor pattern for reaching Qt private members, while also adding new CI workflows to build the project on Deepin (crimson) and Arch Linux.
Changes:
- Added a new header implementing an explicit-template-instantiation-based private member accessor and convenience macros.
- Refactored
DQMLGlobalObjectto accessQSGNodeprivate fields via the new accessor macros instead of#define private public. - Updated scenegraph rendering logic in
dbackdropnode.cppand introduced new GitHub Actions build workflows for Deepin and Arch Linux.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/util/dprivateaccessor_p.h |
Adds the template-based private accessor pattern + macros. |
src/private/dqmlglobalobject.cpp |
Replaces #define private public usage with accessor tags/macros for QSGNode internals. |
src/private/dbackdropnode.cpp |
Removes access-hack macros and updates renderer invocation flow. |
.github/workflows/dtkdeclarative-deepin-build.yml |
Adds Deepin (crimson) container build workflow that builds dependencies from source and produces .deb artifacts. |
.github/workflows/dtkdeclarative-archlinux-build.yml |
Adds Arch Linux container build workflow producing install artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zccrs
force-pushed
the
master
branch
7 times, most recently
from
5579d87 to
a262b4e
Compare
|
|
||
| #include <algorithm> | ||
|
|
||
| #ifndef QT_NO_OPENGL |
There was a problem hiding this comment.
已处理，这个条件编译已经去掉，当前直接保留 static_assert(sizeof(QSGRenderer) == 432)，并已随最新 force-push 一起更新。
|
已更新这个 PR：\n\n1. 代码已经 rebase 到 upstream 最新，并去掉了混入的 fork |
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.
|
补充说明一下刚才的更新：\n\n1. 这个 PR 的 head 用的是 |
deepin pr auto review这份代码的核心改进是将访问 Qt 私有类成员的机制从 然而，在实现细节、跨平台兼容性以及代码安全性上，仍有一些需要严重关注的问题。以下是详细的审查意见： 一、 代码安全与未定义行为1. 严重的内存对齐与类型双关问题 struct TagName##_bf_layout_mirror { PrevMemberType prev; BfStorageType storage; };
static constexpr std::size_t kOffset =
__builtin_offsetof(TagName##_bf_layout_mirror, storage) -
__builtin_offsetof(TagName##_bf_layout_mirror, prev);这里试图通过镜像结构体来计算位域的偏移量，但这是不可靠且危险的：
改进建议：对于位域的访问，最安全的方式是定义一个与原类完全匹配的镜像类（包含相同的位域定义），或者使用编译时静态断言配合手动指定的偏移量。如果必须使用当前方案，必须加上严格的 2. 危险的 static_assert(sizeof(QSGRenderer) == 432, ...);类的尺寸高度依赖于编译器、平台（x86 vs ARM）、以及 Qt 的编译选项。硬编码 改进建议：将其改为范围检查或取消此限制。如果是为了防止 Qt 升级导致布局变化，可以只在 Debug模式下触发 二、 语法与逻辑1. 宏定义中缺少对变参的包裹 #define D_DECLARE_PRIVATE_METHOD(TagName, ClassName, MethodName, RetType, ...) \
struct TagName { \
using MemberPtr = RetType (ClassName::*)(__VA_ARGS__); \
// ...当声明无参方法时， 改进建议：保持当前实现即可，但在文档或注释中提醒使用者，对于含有逗号的复杂类型，需使用别名或括号包裹。 2. 测试用例中的未定义行为 alignas(QSGRenderer) unsigned char buffer[sizeof(QSGRenderer)];
// ...
renderer = reinterpret_cast<QSGRenderer *>(buffer);
// ...
renderer->m_is_rendering = true; // 严重 UB直接在未构造对象的裸内存上调用非静态成员赋值（即使是通过直接访问）在 C++ 标准中是严重的未定义行为。 改进建议：使用 三、 代码质量与性能1. friend constexpr MemberPtr get(TagName) noexcept { return Ptr; }
改进建议：确保所有使用该头文件的翻译单元使用相同的编译器标准设置，或直接将其声明为 2. 测试对编译器扩展的依赖 改进建议：既然测试的目的是验证“通过 Accessor 访问的位域与 Qt 内部真实的位域一致”，更好的测试方法是：在测试中构造一个与 Qt 内部完全相同的镜像 Struct，通过 Accessor 写入，然后通过镜像 Struct 读取，反之亦然。这样就不需要依赖破坏访问控制的编译器选项。 四、 改进后的代码示例针对 // 改进后的 D_DECLARE_PRIVATE_BITFIELD 宏
#define D_DECLARE_PRIVATE_BITFIELD(TagName, ClassName, PrevMember, PrevMemberType, BfStorageType, ExpectedOffset) \
struct TagName { \
using MemberPtr = PrevMemberType ClassName::*; \
friend constexpr MemberPtr get(TagName) noexcept; \
}; \
template struct DtkDeclarativePrivateAccessorImpl<TagName, &ClassName::PrevMember>; \
struct TagName##_Bits { \
/* 使用用户提供的预期偏移量，并在编译期进行强校验 */ \
static constexpr std::size_t kOffset = ExpectedOffset; \
/* 注意：移除了不可靠的 __builtin_offsetof 镜像推算 */ \
static BfStorageType *storagePtr(ClassName *obj) noexcept { \
PrevMemberType &prev_ref = (*obj).*get(TagName{}); \
return reinterpret_cast<BfStorageType*>( \
reinterpret_cast<char *>(&prev_ref) + kOffset); \
} \
static const BfStorageType *storagePtr(const ClassName *obj) noexcept { \
return storagePtr(const_cast<ClassName *>(obj)); \
} \
static bool getBit(const ClassName *obj, unsigned bit) noexcept { \
return (*storagePtr(obj) >> bit) & BfStorageType{1}; \
} \
static void setBit(ClassName *obj, unsigned bit, bool val) noexcept { \
BfStorageType *p = storagePtr(obj); \
if (val) *p |= (BfStorageType{1} << bit); \
else *p &= ~(BfStorageType{1} << bit); \
} \
}在使用处，明确指定偏移量： // 假设通过分析 qsgrenderer_p.h 得知 m_is_rendering 在 m_nodes_dont_preprocess 之后的 0 字节处
D_DECLARE_PRIVATE_BITFIELD(QSGRenderer_m_nodes_dont_preprocess_tag,
QSGRenderer, m_nodes_dont_preprocess,
QSet<QSGNode *>, uint, 0);总结整体架构改进方向非常值得肯定，消除了 |
|
补充一下 GitHub Action 那条提交的处理：当前 PR 仍保持只包含本次修复，不再把 workflow 提交带回 |
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.