Fix Equal and value comparison for double and float types
#3088
Conversation
WalkthroughThis pull request modifies the Changes
Assessment against linked issues
Suggested labels
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/CLR/Core/CLR_RT_HeapBlock.cpp (1)
1657-1722: LGTM! Comprehensive floating-point comparison implementation.The implementation correctly handles all floating-point comparison cases including NaN and infinite values, using safe comparison functions. The code aligns with IEEE 754 floating-point comparison semantics.
Consider adding a comment block explaining the special case handling and post-processing in the interpreter:
+// Special case handling for floating-point comparisons: +// 1. NaN comparisons return 1 (post-processed to 0 in interpreter) +// 2. Infinite value comparisons return 0 (post-processed to 1 in interpreter) +// 3. Regular comparisons use isgreater/isless for safe IEEE 754 compliant comparison case DATATYPE_R4:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CLR/Core/CLR_RT_HeapBlock.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (1)
src/CLR/Core/CLR_RT_HeapBlock.cpp (1)
1334-1335: LGTM! Floating-point types are now correctly handled.The addition of
DATATYPE_R4andDATATYPE_R8to the switch statement ensures that floating-point comparisons are properly handled by routing them through theCompare_Valuesmethod, aligning with the behavior of the full .NET framework.
josesimoes
force-pushed
the
fix-1563
branch
2 times, most recently
from
01a3ff3 to
791d967
Compare
josesimoes
changed the title
Equal and value comparison for double and float types
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/CLR/Core/CLR_RT_HeapBlock.cpp (1)
1104-1119: LGTM! The hash code calculation for floating-point types now correctly handles special cases.The changes ensure that NaN values and both zeros (positive and negative) have the same hash code, aligning with .NET behavior.
Consider adding a reference to the .NET documentation in the comments:
- // ensure that NaN and both zeros have the same hash code + // ensure that NaN and both zeros have the same hash code + // See: https://learn.microsoft.com/en-us/dotnet/api/system.double.gethashcodeAlso applies to: 1130-1143
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/CLR/Core/CLR_RT_HeapBlock.cpp(6 hunks)
🔇 Additional comments (1)
src/CLR/Core/CLR_RT_HeapBlock.cpp (1)
1364-1386: LGTM! The floating-point comparison now correctly handles NaN values.The changes ensure that NaN values are considered equal to each other, aligning with .NET behavior. The implementation correctly handles both single-precision (R4) and double-precision (R8) floating-point types.
The added link to the .NET documentation is particularly helpful for understanding the rationale behind treating NaN values as equal:
https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-double-equals?WT.mc_id=DT-MVP-5004179#nan
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/CLR/Core/CLR_RT_HeapBlock.cpp (3)
1105-1117: LGTM! Consider a minor optimization for hash code generation.The implementation correctly handles special cases for R4 (single-precision) by ensuring that NaN and both zeros (positive and negative) have the same hash code, aligning with .NET behavior.
Consider combining the conditions to reduce branching:
-if ((signBit && (__isnanf(ptr->NumericByRef().r4) || ptr->NumericByRef().r4 == 0))) +if (__isnanf(ptr->NumericByRef().r4) || (signBit && ptr->NumericByRef().r4 == 0))
1130-1143: LGTM! Consider a minor optimization for hash code generation.The implementation correctly handles special cases for R8 (double-precision) by ensuring that NaN and both zeros (positive and negative) have the same hash code, aligning with .NET behavior.
Consider combining the conditions to reduce branching:
-if ((signBit && (__isnand(ptr->NumericByRef().r8) || (double)ptr->NumericByRef().r8 == 0))) +if (__isnand(ptr->NumericByRef().r8) || (signBit && (double)ptr->NumericByRef().r8 == 0))
1364-1386: LGTM! Consider adding documentation for special cases.The implementation correctly handles floating-point comparisons, including the special case where NaN equals NaN, aligning with .NET behavior. The link to the official documentation is a good practice.
Consider adding a comment block to explain the special cases:
// edge cases, in .NET a NaN is equal to another NaN // https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-double-equals?WT.mc_id=DT-MVP-5004179#nan +// Special cases for floating-point comparison: +// 1. NaN == NaN returns true (unlike IEEE 754) +// 2. -0.0 == +0.0 returns true +// 3. All other comparisons follow IEEE 754
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/CLR/Core/CLR_RT_HeapBlock.cpp(6 hunks)src/PAL/Include/nanoPAL_NativeDouble.h(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (1)
src/PAL/Include/nanoPAL_NativeDouble.h (1)
16-17: Verify the correctness of using_isnanfor both single and double precision.The implementation reuses
_isnanfor both single-precision (__isnanf) and double-precision (__isnand) NaN checks. While this might work in most cases, it's important to verify that_isnancorrectly handles both precisions.
- Add data types to switch that performs value comparison. - Fix comparison of NaN values for both double and float (C types).
Automated fixes for code style.
Automated fixes for code style.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/CLR/Core/CLR_RT_HeapBlock.cpp (3)
1104-1119: Use__signbitffor float type instead of__signbitd.While the implementation correctly handles NaN and zero values for R4 type, it's using
__signbitdwhich is meant for double precision. For better type safety and clarity, use__signbitffor float type.- int signBit = __signbitd(ptr->NumericByRef().r4); + int signBit = __signbitf(ptr->NumericByRef().r4);
1130-1143: Remove redundant casts to double.The implementation correctly handles NaN and zero values for R8 type. However, there are redundant casts to double as
r8is already a double precision value.- int signBit = __signbitd((double)ptr->NumericByRef().r8); + int signBit = __signbitd(ptr->NumericByRef().r8); - if (__isnand((double)ptr->NumericByRef().r8) || (signBit && (double)ptr->NumericByRef().r8 == 0)) + if (__isnand(ptr->NumericByRef().r8) || (signBit && ptr->NumericByRef().r8 == 0))
1376-1386: Remove redundant casts to double in R8 comparison.The implementation correctly handles NaN equality for R8 type. However, there are redundant casts to double that can be removed.
- if (__isnand((double)pArgLeft.NumericByRefConst().r8) && __isnand((double)pArgRight.NumericByRefConst().r8)) + if (__isnand(pArgLeft.NumericByRefConst().r8) && __isnand(pArgRight.NumericByRefConst().r8))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/CLR/Core/CLR_RT_HeapBlock.cpp(6 hunks)src/PAL/Include/nanoPAL_NativeDouble.h(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/PAL/Include/nanoPAL_NativeDouble.h
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (1)
src/CLR/Core/CLR_RT_HeapBlock.cpp (1)
1364-1375: LGTM! Correct implementation of NaN equality for R4.The implementation correctly handles NaN equality for R4 type, aligning with .NET behavior where NaN values are considered equal to other NaN values. The code is well-documented with a reference to official documentation.
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
NaNvalues, ensuring consistent behavior in numerical operations.Chores