Skip to content

fix: Enhance compression options and ARM CRC32 support#368

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:develop/snipefrom
dengzhongyuan365-dev:pzip-cpp
Mar 3, 2026
Merged

fix: Enhance compression options and ARM CRC32 support#368
deepin-bot[bot] merged 1 commit intolinuxdeepin:develop/snipefrom
dengzhongyuan365-dev:pzip-cpp

Conversation

@dengzhongyuan365-dev
Copy link
Contributor

  • Updated the CliPzipPlugin to allow dynamic compression levels (0-9) instead of a fixed level.
  • Introduced a 'streamFromSource' option in FileTask for direct reading from source files in Store mode.
  • Improved CRC32 calculation for ARM architecture with hardware support detection.
  • Adjusted ZipWriter to ensure accurate header updates after data writing.

This enhances flexibility in compression settings and optimizes performance on ARM platforms.

bug: https://pms.uniontech.com/bug-view-310401.html

- Updated the CliPzipPlugin to allow dynamic compression levels (0-9) instead of a fixed level.
- Introduced a 'streamFromSource' option in FileTask for direct reading from source files in Store mode.
- Improved CRC32 calculation for ARM architecture with hardware support detection.
- Adjusted ZipWriter to ensure accurate header updates after data writing.

This enhances flexibility in compression settings and optimizes performance on ARM platforms.

bug: https://pms.uniontech.com/bug-view-310401.html
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Sorry @dengzhongyuan365-dev, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@deepin-ci-robot
Copy link

deepin pr auto review

这份代码修改涉及多个方面,包括压缩级别控制、ARM平台优化、Store模式实现以及CRC32计算优化。下面我将从语法逻辑、代码质量、代码性能和代码安全四个方面进行详细审查。

1. 语法逻辑

优点:

  • 代码逻辑清晰,修改点明确
  • 使用了 C++17 的 std::clamp 来限制压缩级别范围,符合现代 C++ 风格
  • 在 ARM 平台上正确使用了条件编译和硬件特性检测

问题和建议:

  1. clipzipplugin.cpp 中的压缩级别处理

    int level = options.iCompressionLevel;
    if (level < 0 || level > 9) {
        level = 1;
    }
    • 建议使用 std::clamp 保持一致性:
      int level = std::clamp(options.iCompressionLevel, 0, 9);
    • 原代码将无效值设为1,但ZIP规范中0是有效的Store模式,建议改为:
      int level = std::clamp(options.iCompressionLevel, 0, 9);
      if (level < 0 || level > 9) {
          level = 0; // 默认使用Store模式
      }
  2. archiver.cpp 中的压缩级别转换

    auto level = static_cast<CompressionLevel>(
        std::clamp(options_.compressionLevel, 1, 9));
    • 这里将范围限制为1-9,但前面已经处理了0的情况,逻辑上没问题,但建议明确注释说明为什么这里排除0

2. 代码质量

优点:

  • 添加了有意义的注释
  • 正确处理了移动构造函数和移动赋值运算符中的新成员变量
  • 使用了 RAII 风格的资源管理

问题和建议:

  1. file_task.cpp 中的文件写入检查

    if (overflow_ && overflow_->is_open()) {
        overflow_->write(reinterpret_cast<const char*>(data), size);
        if (overflow_->good()) {
            totalWritten += size;
        }
    }
    • 建议在写入失败时返回错误信息,而不是静默忽略:
      if (overflow_ && overflow_->is_open()) {
        overflow_->write(reinterpret_cast<const char*>(data), size);
        if (overflow_->good()) {
            totalWritten += size;
        } else {
            // 记录错误或返回错误码
            return totalWritten; // 或其他错误处理
        }

    }

    
    
  2. file_task.cpp 中的文件读取

    std::ifstream file(path, std::ios::binary);
    if (!file.is_open()) return;
    • 建议添加错误处理:
      std::ifstream file(path, std::ios::binary);
      if (!file.is_open()) {
          // 记录错误或设置错误状态
          return;
      }
  3. zip_writer.cpp 中的数据处理顺序

    • CentralDirEntry entry 的创建移到 dataProvider 之后是正确的,但建议添加注释说明为什么这样做:
      // dataProvider 运行后再拷贝 header,确保 CRC 等字段是最新值
      CentralDirEntry entry;
      entry.header = header;
      entry.localHeaderOffset = localHeaderOffset;
      centralDir_.push_back(entry);

3. 代码性能

优点:

  • ARM 平台上使用了硬件 CRC32 指令,显著提升性能
  • Store 模式下直接流式读取源文件,避免不必要的内存拷贝
  • 使用了循环展开技术

问题和建议:

  1. ARM CRC32 实现优化

    while (size >= 64) {
        c = __crc32d(c, *reinterpret_cast<const uint64_t*>(data));
        // ... 7 more lines
    }
    • 已经进行了很好的循环展开,但可以考虑使用 NEON 指令进一步优化:
      #ifdef __ARM_NEON
      // 使用 NEON 指令处理更大的块
      #endif
  2. 缓冲区大小

    constexpr size_t BUFFER_SIZE = 32 * 1024;
    • 建议根据系统特性动态调整缓冲区大小,或者使用更大的缓冲区(如 64KB 或 128KB)以减少 I/O 操作
  3. 文件读取优化

    • readCompressedData 中,可以考虑使用内存映射文件(mmap)来提高大文件的读取性能

4. 代码安全

优点:

  • 对压缩级别进行了范围检查
  • 使用了类型安全的转换
  • 正确处理了文件 I/O 错误

问题和建议:

  1. 类型转换

    c = __crc32d(c, *reinterpret_cast<const uint64_t*>(data));
    • 建议添加对齐检查,避免未对齐访问:
      if (reinterpret_cast<uintptr_t>(data) & 7) {
          // 处理未对齐情况
      }
  2. 文件路径处理

    • 建议对文件路径进行规范化处理,防止路径遍历攻击:
      fs::path safePath = fs::canonical(path);
  3. 资源管理

    • file_task.cpp 中,overflow_ 的重置操作应该在所有错误路径上都执行:
      if (!overflow_->is_open()) {
          overflow_.reset();
          return totalWritten;
      }
  4. 整数溢出检查

    • 在计算文件大小和偏移量时,建议添加溢出检查:
      if (totalBytesRead > std::numeric_limits<uint64_t>::max() - size) {
          // 处理溢出
      }

5. 其他建议

  1. 错误处理一致性

    • 建议在整个项目中统一错误处理方式,例如都使用 Error 类型或异常
  2. 日志记录

    • 建议在关键操作处添加日志记录,便于调试和问题追踪
  3. 单元测试

    • 建议为新增的 Store 模式和 ARM CRC32 实现添加单元测试
  4. 文档更新

    • 建议更新相关文档,说明新增的 Store 模式和 ARM 平台优化

总结

这份代码修改整体质量较高,主要优化了压缩级别控制和 ARM 平台性能,并实现了 Store 模式。主要改进点包括:

  1. 增强了压缩级别的控制,支持 Store 模式
  2. 优化了 ARM 平台上的 CRC32 计算性能
  3. 实现了 Store 模式下的流式处理,减少内存使用
  4. 改进了文件 I/O 的错误处理

建议在后续版本中进一步完善错误处理、增加日志记录,并考虑使用更高级的性能优化技术(如 NEON 指令、内存映射文件等)。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dengzhongyuan365-dev, lzwind

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

@dengzhongyuan365-dev
Copy link
Contributor Author

/merge

@deepin-bot deepin-bot bot merged commit e2a38c9 into linuxdeepin:develop/snipe Mar 3, 2026
16 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