Qute: if section - adjust the evaluation rules for equality operators #44615
Conversation
mkouba
commented
Nov 21, 2024
mkouba
commented
- fixes Qute: Unexpected primitive equality #44610
|
@neon-dev Hopefully this makes a little bit more sense. |
| boolean equals(Object op1, Object op2) { | ||
| if (op1 == op2) { | ||
| return true; | ||
| } | ||
| if (op1 != null && op2 != null && (op1 instanceof Number || op2 instanceof Number)) { | ||
| if (op1.getClass().equals(op2.getClass())) { | ||
| // Both operands are of the same Number type | ||
| return op1.equals(op2); | ||
| } | ||
| // Both operands are not null and at least one of them is a number | ||
| return getDecimal(op1).compareTo(getDecimal(op2)) == 0; | ||
| } | ||
| return Objects.equals(op1, op2); | ||
| } |
There was a problem hiding this comment.
Looks good, thanks a lot!
Just one question:
Couldn't the following code be simplified like so?
| boolean equals(Object op1, Object op2) { | |
| if (op1 == op2) { | |
| return true; | |
| } | |
| if (op1 != null && op2 != null && (op1 instanceof Number || op2 instanceof Number)) { | |
| if (op1.getClass().equals(op2.getClass())) { | |
| // Both operands are of the same Number type | |
| return op1.equals(op2); | |
| } | |
| // Both operands are not null and at least one of them is a number | |
| return getDecimal(op1).compareTo(getDecimal(op2)) == 0; | |
| } | |
| return Objects.equals(op1, op2); | |
| } | |
| boolean equals(Object op1, Object op2) { | |
| if (Objects.equals(op1, op2)) { | |
| return true; | |
| } | |
| if (op1 instanceof Number && op2 instanceof Number) { | |
| return getDecimal(op1).compareTo(getDecimal(op2)) == 0; | |
| } | |
| return false; | |
| } |
This check would not throw a TemplateException if only one of the operands is instanceof Number, which may or may not be wanted.
There was a problem hiding this comment.
Hm yes, well, your proposal is not functionally equivalent. The current version allows for comparison of e.g. String and Integer. Also op1 == op2 should be, in theory, more effective than equals(). In other words, we only want to call equals unless the objects are identical or number comparison is possible. Does that make sense?
There was a problem hiding this comment.
The current version allows for comparison of e.g. String and Integer.
Oh yeah, I completely missed that. So {#if 0 == "0"} should be true if I understand correctly?
we only want to call equals unless the objects are identical or number comparison is possible
Objects.equals is implemented as (a == b) || (a != null && a.equals(b));, so we would only see unnecessary .equals calls for two objects of different Number types.
There was a problem hiding this comment.
The current version allows for comparison of e.g. String and Integer.
Oh yeah, I completely missed that. So
{#if 0 == "0"}should betrueif I understand correctly?
Yes, I think so. We do the same for comparison operators such as >; e.g. {#if 10 > "1"} should be also true.
we only want to call equals unless the objects are identical or number comparison is possible
Objects.equalsis implemented as(a == b) || (a != null && a.equals(b));, so we would only see unnecessary.equalscalls for two objects of different Number types.
Yes.
There was a problem hiding this comment.
Got it. So this would be functionally equivalent, but maybe not much of an improvement anymore:
boolean equals(Object op1, Object op2) {
if (Objects.equals(op1, op2)) {
return true;
}
if (op1 != null && op2 != null && (op1 instanceof Number || op2 instanceof Number)) {
return getDecimal(op1).compareTo(getDecimal(op2)) == 0;
}
return false;
}
|
🎊 PR Preview 16759d4 has been successfully built and deployed to https://quarkus-pr-main-44615-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
independent-projects/qute/core/src/main/java/io/quarkus/qute/IfSectionHelper.java
Outdated
Show resolved
Hide resolved
Status for workflow
|
Status for workflow
|
