-
Notifications
You must be signed in to change notification settings - Fork 61
Revert "fix: 10% should not rely on the preceding numerical values." #191
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
Revert "fix: 10% should not rely on the preceding numerical values." #191
Conversation
This reverts commit 31fab1d.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR reverts a previous behavioral change to percentage evaluation in the core evaluator, restoring the use of the preceding value (val2) when computing percentages. Sequence diagram for percentage evaluation restored behaviorsequenceDiagram
participant Evaluator
participant Stack_Quantity as Stack
participant Quantity
participant HNumber
Evaluator->>Stack: pop() to get val1 (current value)
Evaluator->>Stack: peek() or previous result to get val2
%% New (restored) behavior
Evaluator->>HNumber: create("0.01")
HNumber-->>Evaluator: hNum_001
Evaluator->>Quantity: multiply(val1, hNum_001)
Quantity-->>Evaluator: percentBase
Evaluator->>Quantity: multiply(val2, percentBase)
Quantity-->>Evaluator: percentResult
Evaluator->>Evaluator: checkOperatorResult(percentResult)
Evaluator->>Stack: push(percentResult)
Evaluator->>Evaluator: m_standardPercent = percentResult
Class diagram for Evaluator percentage handling in execclassDiagram
class Evaluator {
+exec(opcodes QVector_Opcode, stack Stack_Quantity) Quantity
-m_standardPercent Quantity
}
class Quantity
class HNumber
class Opcode
class QVector_Opcode
class Stack_Quantity
Evaluator --> Quantity
Evaluator --> HNumber
Evaluator --> QVector_Opcode
Evaluator --> Stack_Quantity
%% Focus on the changed percentage computation inside exec
class PercentageEvaluationContext {
+val1 Quantity
+val2 Quantity
+applyPercent(val1 Quantity, val2 Quantity) Quantity
}
Evaluator ..> PercentageEvaluationContext : uses
%% Old vs new behavior (conceptual methods)
PercentageEvaluationContext : +applyPercent_old(val1 Quantity) Quantity
PercentageEvaluationContext : +applyPercent_new(val1 Quantity, val2 Quantity) Quantity
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 left some high level feedback:
- Since this reintroduces the prior percent behavior that was recently changed, consider adding a brief comment near this logic describing the intended semantics of
%(especially howval2is used) to avoid future flip-flop changes here.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since this reintroduces the prior percent behavior that was recently changed, consider adding a brief comment near this logic describing the intended semantics of `%` (especially how `val2` is used) to avoid future flip-flop changes here.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
deepin pr auto review以下是对这段代码变更的详细审查和改进建议: 1. 语法逻辑审查当前变更分析:
潜在问题:
2. 代码质量审查
3. 代码性能审查
4. 代码安全审查
改进建议
最终改进代码示例static const HNumber PERCENT_FACTOR("0.01");
// 绝对值模式:10% = 0.1,10% + 10% = 0.2
val1 = checkOperatorResult(val1 * PERCENT_FACTOR);
m_standardPercent = val1;
stack.push(val1);
break;或(相对值模式): static const HNumber PERCENT_FACTOR("0.01");
// 相对值模式:100 + 10% = 110(即 100 * 0.1 + 100)
val1 = checkOperatorResult(val2 * val1 * PERCENT_FACTOR);
m_standardPercent = val1;
stack.push(val1);
break;总结
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lichaofan2008, max-lvs 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 |
|
/merge |
This reverts commit 31fab1d.
Summary by Sourcery
Bug Fixes: