Skip to content

Conversation

@18202781743
Copy link
Contributor

  1. Added version macros (VERSION_MAJOR/VERSION_MINOR) to generated
    header files for version tracking
  2. Added property definition macros for all configuration properties to
    facilitate caller compatibility
  3. Added version compatibility check in dconfig2cpp tool to warn when
    JSON file version exceeds supported version
  4. Updated header comment to include dconfig2cpp tool version
    information
  5. Added version struct and supported version constant to main.cpp
  6. Modified code generation to output property macros before class
    definition

Log: Added version and property macros to dconfig2cpp generated files
for better compatibility

Influence:

  1. Test dconfig2cpp tool with JSON files of different versions (older,
    same, newer)
  2. Verify generated header files contain correct version macros
  3. Check that property definition macros are generated for all
    configuration properties
  4. Test compilation of projects using the generated header files
  5. Verify backward compatibility with existing code using the generated
    classes
  6. Test version warning mechanism when using newer JSON files

feat: dconfig2cpp添加版本宏和属性定义宏

  1. 在生成的头文件中添加版本宏(VERSION_MAJOR/VERSION_MINOR)用于版本跟踪
  2. 为所有配置属性添加属性定义宏,方便调用方兼容性处理
  3. 在dconfig2cpp工具中添加版本兼容性检查,当JSON文件版本超过支持版本时发
    出警告
  4. 更新头文件注释以包含dconfig2cpp工具版本信息
  5. 在main.cpp中添加版本结构和支持的版本常量
  6. 修改代码生成逻辑,在类定义之前输出属性宏

Log: 为dconfig2cpp生成的文件添加版本和属性宏,提高兼容性

Influence:

  1. 测试dconfig2cpp工具处理不同版本(旧版、相同、新版)的JSON文件
  2. 验证生成的头文件包含正确的版本宏
  3. 检查是否为所有配置属性生成了属性定义宏
  4. 测试使用生成头文件的项目的编译情况
  5. 验证与现有使用生成类的代码的向后兼容性
  6. 测试使用新版JSON文件时的版本警告机制

1. Added version macros (VERSION_MAJOR/VERSION_MINOR) to generated
header files for version tracking
2. Added property definition macros for all configuration properties to
facilitate caller compatibility
3. Added version compatibility check in dconfig2cpp tool to warn when
JSON file version exceeds supported version
4. Updated header comment to include dconfig2cpp tool version
information
5. Added version struct and supported version constant to main.cpp
6. Modified code generation to output property macros before class
definition

Log: Added version and property macros to dconfig2cpp generated files
for better compatibility

Influence:
1. Test dconfig2cpp tool with JSON files of different versions (older,
same, newer)
2. Verify generated header files contain correct version macros
3. Check that property definition macros are generated for all
configuration properties
4. Test compilation of projects using the generated header files
5. Verify backward compatibility with existing code using the generated
classes
6. Test version warning mechanism when using newer JSON files

feat: dconfig2cpp添加版本宏和属性定义宏

1. 在生成的头文件中添加版本宏(VERSION_MAJOR/VERSION_MINOR)用于版本跟踪
2. 为所有配置属性添加属性定义宏,方便调用方兼容性处理
3. 在dconfig2cpp工具中添加版本兼容性检查,当JSON文件版本超过支持版本时发
出警告
4. 更新头文件注释以包含dconfig2cpp工具版本信息
5. 在main.cpp中添加版本结构和支持的版本常量
6. 修改代码生成逻辑,在类定义之前输出属性宏

Log: 为dconfig2cpp生成的文件添加版本和属性宏,提高兼容性

Influence:
1. 测试dconfig2cpp工具处理不同版本(旧版、相同、新版)的JSON文件
2. 验证生成的头文件包含正确的版本宏
3. 检查是否为所有配置属性生成了属性定义宏
4. 测试使用生成头文件的项目的编译情况
5. 验证与现有使用生成类的代码的向后兼容性
6. 测试使用新版JSON文件时的版本警告机制
@deepin-ci-robot
Copy link
Contributor

deepin pr auto review

这份代码审查将针对提供的 git diff 进行,主要关注 dconfig2cpp 工具的更新及其生成的头文件。审查将从语法逻辑、代码质量、代码性能和代码安全四个方面展开。

1. 语法逻辑

  • 版本号解析逻辑:

    • 代码: const auto versionParts = version.split('.'); if (versionParts.size() == 2) { ... }
    • 问题: 逻辑过于严格。如果版本号是 1.0.1(包含补丁号)或者仅仅是 1,这段代码会将 fileMajorVersion 留空,导致后续的版本检查失败(if (fileMajorVersion != "1")),进而导致程序退出并返回错误。
    • 建议: 修改版本解析逻辑,使其更加健壮。应优先获取第一个部分作为主版本号。
    • 改进代码:
      const auto versionParts = version.split('.');
      if (!versionParts.isEmpty()) {
          fileMajorVersion = versionParts.first();
      }
      if (fileMajorVersion != "1") {
          qWarning() << "Warning: The JSON file version isn't supported. Expected major version 1, got:" << fileMajorVersion;
          return -1;
      }
  • 宏定义的命名:

    • 代码: #define DCONFIG_ORG_DEEPIN_DTK_PREFERENCE_DCONFIG_FILE_autoDisplayFeature
    • 问题: 生成的宏名直接使用了 JSON 的 key(如 autoDisplayFeature)。如果 key 包含特殊字符或以数字开头,生成的宏定义将不符合 C++ 标识符规范,导致编译错误。
    • 建议: 在生成宏定义之前,对 key 进行合法性检查或转换(例如,将非字母数字字符替换为下划线,或者确保不以数字开头)。

2. 代码质量

  • 魔法数字与硬编码:

    • 代码: if (fileMajorVersion != "1") 以及 static constexpr Version ToolVersion{1, 1};
    • 问题: 版本号 "1" 被硬编码在逻辑判断中。
    • 建议: 虽然当前工具只支持版本 1,但为了代码的可维护性,建议定义一个常量表示支持的最低或目标主版本号,例如 static constexpr quint16 SupportedJsonMajorVersion = 1;,并在比较时使用该常量。
  • 注释与文档:

    • 代码: // Queue state transition to main thread, to ensure config values are fully initialized.
    • 评价: 这是一个很好的改进。添加注释解释了为什么使用 Qt::QueuedConnection,这有助于后续维护者理解线程间通信的意图。
  • 头文件生成结构:

    • 代码: 在头文件中插入了大量的 #define 宏。
    • 问题:
      1. 命名空间污染: 这些宏没有前缀保护(虽然类名很长,但依然存在风险),直接定义在全局作用域。
      2. 宏定义值: 生成的宏如 #define DCONFIG_..._autoDisplayFeature 没有赋值(即定义为空)。虽然这可以用作 #ifdef 检查,但通常宏定义用于表示常量。如果目的是用于 #ifdef,建议在注释中明确说明。
    • 建议: 考虑将这些宏定义包裹在一个特定的命名空间中,或者使用 enum 来替代部分宏,以减少全局命名空间的污染。如果必须使用宏,确保宏名足够唯一。

3. 代码性能

  • 字符串处理:

    • 代码: QString version = root[QLatin1String("version")].toString(); 以及 QString("%1.%2").arg(ToolVersion.major).arg(ToolVersion.minor)
    • 评价: 使用 QLatin1String 是好的习惯。QString::arg 在此处用于生成注释,执行频率极低,对性能影响可忽略不计。无需优化。
  • 版本号分割:

    • 代码: version.split('.')
    • 评价: 同样,由于仅在程序启动时执行一次,性能不是瓶颈。

4. 代码安全

  • 输入验证:

    • 代码: JSON 解析后的版本号检查。
    • 问题: 目前只检查了主版本号是否为 "1"。如果 JSON 文件被篡改或格式错误(例如 version 字段不是字符串),toString() 可能返回空字符串或非预期格式。
    • 建议: 增加对 JSON 结构的验证。例如,检查 version 字段是否存在且类型为字符串。如果版本格式完全不可解析,应给出明确的错误提示。
  • 宏定义的安全性:

    • 代码: headerStream << "#define " << className.toUpper() << "_DCONFIG_FILE_" << propName << "\n";
    • 问题: 如前所述,直接拼接 propName 存在安全风险。如果配置文件由不可信来源提供,或者 key 名包含恶意构造的字符(如换行符 //),可能会破坏生成的头文件结构,甚至引入代码注入风险。
    • 建议: 必须对 propName 进行过滤或转义,确保它只包含合法的 C++ 标识符字符(字母、数字、下划线),且不以数字开头。

总结与改进建议

总体来看,这次改动增加了工具版本信息的记录和生成代码的宏定义,有助于版本追踪和条件编译。但在处理输入(版本号和配置项名称)时的健壮性有待加强。

关键修改建议总结:

  1. 修复版本解析逻辑:不要强制要求版本号必须包含两个部分,应只取第一部分作为主版本号。
  2. 增强宏名生成安全性:过滤或转义配置项名称,防止生成非法的 C++ 标识符。
  3. 输入验证:在解析 JSON 前或后,增加对关键字段类型和格式的检查。

针对 tools/dconfig2cpp/main.cpp 的具体修改建议代码片段:

// 建议在 main.cpp 中添加一个辅助函数来清理属性名,用于生成宏
QString sanitizeMacroName(const QString &input) {
    QString result;
    // 简单的过滤逻辑:只保留字母数字和下划线
    // 注意:这只是示例,可能需要更复杂的逻辑来处理开头是数字的情况
    for (const QChar &ch : input) {
        if (ch.isLetterOrNumber() || ch == '_') {
            result.append(ch);
        } else {
            result.append('_'); // 将非法字符替换为下划线
        }
    }
    // 确保不以数字开头
    if (!result.isEmpty() && result[0].isDigit()) {
        result.prepend('_');
    }
    return result;
}

// 在生成宏的地方使用
for (const QString &propName : allPropertyNames) {
    QString safeName = sanitizeMacroName(propName);
    headerStream << "#define " << className.toUpper() << "_DCONFIG_FILE_" << safeName << "\n";
}

针对版本检查逻辑的修改建议代码片段:

    // Parse version to get major
    QString fileMajorVersion;
    const auto versionParts = version.split('.');
    
    // 逻辑改进:只要不为空就取第一部分
    if (!versionParts.isEmpty()) {
        fileMajorVersion = versionParts.first();
    }

    // 检查是否为空或不是数字(简单的健壮性检查)
    bool ok;
    int majorVer = fileMajorVersion.toInt(&ok);
    
    if (!ok || majorVer != 1) {
        qWarning() << QLatin1String("Warning: The JSON file version isn't supported. Expected major version 1.");
        return -1;
    }

@deepin-ci-robot
Copy link
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 59ee9fc into linuxdeepin:master Jan 22, 2026
17 of 19 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.

3 participants