-
Notifications
You must be signed in to change notification settings - Fork 37
fix: [build] build error #339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
control the version of watermark Log: as title
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the DTK version range used to detect the custom watermark API to avoid a build-time mismatch with newer DTK versions, while documenting the special-case version that still provides the struct-based watermark interface. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- The updated condition
DTK_VERSION < DTK_VERSION_CHECK(5, 6, 8, 10)excludes 5.6.8.10, which contradicts the new comment stating that 5.6.8.10 also has the interface; consider adjusting the comparison (e.g.,<= 5.6.8.10or keeping< 5.6.9.0) so the code matches the intent. - To make this version logic easier to maintain and reason about, consider extracting the DTK version range into a helper function or named constants instead of embedding the boundary checks directly in the preprocessor condition.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The updated condition `DTK_VERSION < DTK_VERSION_CHECK(5, 6, 8, 10)` excludes 5.6.8.10, which contradicts the new comment stating that 5.6.8.10 also has the interface; consider adjusting the comparison (e.g., `<= 5.6.8.10` or keeping `< 5.6.9.0`) so the code matches the intent.
- To make this version logic easier to maintain and reason about, consider extracting the DTK version range into a helper function or named constants instead of embedding the boundary checks directly in the preprocessor condition.
## Individual Comments
### Comment 1
<location> `src/main.cpp:65-66` </location>
<code_context>
// 主线水印接口在 5.6.9 之后引入.
- // 因此,判断定制线:存在水印接口,版本不低于 5.4.42.7 且低于 5.6.9
-#if DTK_VERSION_CHECK(5, 4, 42, 7) <= DTK_VERSION && DTK_VERSION < DTK_VERSION_CHECK(5, 6, 9, 0)
+ // 因此,判断定制线:存在水印接口,版本不低于 5.4.42.7 且低于 5.6.9(部分镜像上存在5.6.8.10版本,也存在该接口)
+#if DTK_VERSION_CHECK(5, 4, 42, 7) <= DTK_VERSION && DTK_VERSION < DTK_VERSION_CHECK(5, 6, 8, 10)
//接口变动为结构体
data.text = mapwaterMark.value("text").toString();
</code_context>
<issue_to_address>
**issue (bug_risk):** Version range condition likely excludes 5.6.8.10 despite the comment claiming support.
The condition `DTK_VERSION < DTK_VERSION_CHECK(5, 6, 8, 10)` excludes 5.6.8.10, even though the comment says this version has the watermark interface. Previously, `< DTK_VERSION_CHECK(5, 6, 9, 0)` included 5.6.8.10. If 5.6.8.10 should be handled, consider `<= DTK_VERSION_CHECK(5, 6, 8, 10)` or otherwise adjust the bounds so they match the intended supported versions and the comment.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
/merge |
control the version of watermark
Log: as title
Summary by Sourcery
Adjust watermark feature version range to maintain compatibility with specific DTK versions.
Bug Fixes:
Build: