Comparison operator (== and !=) on the JSON::Variant object #1974
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds equality comparison operators (operator== and operator!=) to JSON-related classes in Thunder's Core library, enabling value-based comparison of ArrayType, Variant, and VariantContainer objects.
Key Changes:
- Implemented
operator==andoperator!=forArrayType<ELEMENT>template class - Implemented
operator==andoperator!=forVariantclass with type-specific comparisons - Implemented
operator==andoperator!=forVariantContainerclass with key-value pair comparison - Added helper method
FindValue()toVariantContainerfor key-based value lookup
Comments suppressed due to low confidence (1)
Source/core/JSON.h:1
- The newly added equality operators for Variant lack test coverage. Consider adding unit tests to verify correct behavior for all variant types (EMPTY, BOOLEAN, STRING, NUMBER, FLOAT, DOUBLE, ARRAY, OBJECT), including edge cases and type mismatches.
/*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case type::OBJECT: | ||
| return Object() == other.Object(); |
There was a problem hiding this comment.
Comparing OBJECT types by calling Object() on both sides performs string serialization and deserialization (via ToString() and FromString() as seen in lines 4969-4973), which is expensive. Consider caching the deserialized VariantContainer or implementing a more efficient comparison strategy.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| switch (_type) { | ||
| case type::EMPTY: | ||
| return true; | ||
| case type::BOOLEAN: | ||
| return Boolean() == other.Boolean(); | ||
| case type::STRING: | ||
| return String() == other.String(); | ||
| case type::NUMBER: | ||
| return Number() == other.Number(); | ||
| case type::FLOAT: | ||
| return Float() == other.Float(); | ||
| case type::DOUBLE: | ||
| return Double() == other.Double(); | ||
| case type::ARRAY: | ||
| return Array() == other.Array(); | ||
| case type::OBJECT: | ||
| return Object() == other.Object(); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
The switch statement lacks a default case. While all enum values are covered, adding a default case that either asserts or returns false would make the code more robust against future enum additions and satisfy compiler warnings with -Wswitch-default.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case type::OBJECT: | ||
| return Object() == other.Object(); | ||
| default: | ||
| ASSERT(false); |
There was a problem hiding this comment.
The default case contains an ASSERT(false) but still falls through to return false. Consider adding an explicit return statement after the ASSERT, or use ASSERT_UNREACHABLE() if available, to make the intended control flow clearer and avoid compiler warnings about missing return in some configurations.
| ASSERT(false); | |
| ASSERT(false); | |
| return false; |
| return Float() == other.Float(); | ||
| case type::DOUBLE: | ||
| return Double() == other.Double(); |
There was a problem hiding this comment.
Direct floating-point equality comparison may produce unexpected results due to precision issues. Consider using an epsilon-based comparison for Float() and Double() types, or document that exact bitwise equality is intentional if that's the desired behavior.
2 participants
No description provided.