THRIFT-6075: Generate Equal method for Delphi Thrift structs#3612
Draft
Jens-G wants to merge 4 commits into
Draft
THRIFT-6075: Generate Equal method for Delphi Thrift structs#3612Jens-G wants to merge 4 commits into
Jens-G wants to merge 4 commits into
Conversation
Client: delphi Patch: Jens Geyer Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ssion tests Client: delphi The set/map equality scan in generate_equal_container checked only that every lhs element had some match in rhs, without marking matched rhs entries as consumed. One rhs element could satisfy multiple lhs elements, producing a false Equal=True when one side held reference-distinct but value-equal elements (e.g. two struct-typed set elements with the same field values, reachable because the generated collections use no custom IEqualityComparer and Delphi's default comparer for interface-typed elements is reference-based). Both the set and map branches now match against an indexed copy of rhs plus a parallel used-flags array, marking entries consumed on match. Adds EqualityItem/EqualityItemContainers fixtures to SimpleException.thrift (Empty has no fields and can't express two distinct-but-equal instances) and set/map duplicate-value and duplicate-key regression cases to Test_Equal, alongside a sanity check for the legitimate-equal case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ew code Client: delphi generate_delphi_struct_equality_impl took an is_exception parameter whose only call site always passed false; the true branch was unreachable (exceptions get their equality via a separate hand-written Equals(TObject) wrapper). Removed the parameter and the dead branch. Ran git clang-format scoped to the lines changed since 289a1c1, rather than a full-file reformat, since this file was not clang-format-conformant before this PR and a full pass would have produced an unrelated diff. Verified both changes are no-ops: rebuilt the compiler and regenerated test/DebugProtoTest.thrift (default and com_types) before and after, output is byte-identical. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Client: delphi Three changes to generate_equal_container, none of which alter the comparison semantics: 1. Capture operands into locals: the recursion previously passed composed access-path strings down, so expressions like Self.List_field[_i][_k][_k2] (property getter + GetItem + two hash lookups) were re-evaluated at every mention, up to ~8x per loop iteration in deeply nested containers. Operands are now captured into fresh locals once per level (already_local skips the copy where the operand is a loop variable, pair field or snapshot array slot). 2. Compare maps via pair enumeration and TryGetValue: the scalar-key path iterates lhs pairs and fetches the rhs value with a single lookup (was three: ContainsKey + lhs[k] + rhs[k], plus repeated getters). The non-scalar-key consume-scan snapshots rhs keys and values into parallel arrays, removing hash lookups from that path entirely. 3. Exit(False) directly on mismatch: a new exit_on_fail mode lets field-level, list-element and scalar-key-map comparisons leave the generated function immediately instead of threading _eq flags and break statements through every nesting level. The flag-and-fall- through form remains only for the candidate probes inside the set/map consume-scans, where a mismatch means "try the next candidate" rather than "structs differ". Verified by rebuilding the compiler and regenerating ThriftTest, DebugProtoTest and SimpleException in default, com_types and rtti modes: all 358 generated Equal/Equals functions are begin/end-balanced, structs without containers/binaries generate byte-identical code, and the duplicate-element consume-scan regression shape is preserved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
function Equal(const other: I{Name}): Booleanto every generated Delphi struct interface and its implementation classfunction Equals(Obj: TObject): Booleanon the implementation class to delegate toEqualvia interface checkEquals(Obj: TObject)on generated exception wrapper classes (T{Name}) to compare the innerFDatastruct__issetsemantics for optional fields and recursing into all container/struct typesDetail
The generator gains two new methods:
generate_equal_container— recursive helper that emits comparison code for any field type into a pair ofostringstreambuffers (codeandvars). Uses the boolean-variable pattern (_eqN := True; ... if not _eqN then Exit(False)) so comparison never usesExitinside loops, supporting arbitrary nesting depth. Strategies per type:<>comparisonTBytes/IThriftBytes): length check thenSystem.SysUtils.CompareMem.Equal()recursive callContains()(O(n), value-hash equality).Equal()on each pairContainsKey()+ value comparisongenerate_delphi_struct_equality_impl— walks all struct fields. For optional/unqualified fields emits__issetguard first (mismatch ⇒Exit(False)); required fields compare values directly with no isset check.Tests
Extended
TTestSerializer.Test_Equalinlib/delphi/test/serializer/TestSerializer.Tests.pas:Equals(TObject)compatible type, value mismatch, incompatible type (TBonkImpl)Test plan
TTestSerializer.Test_Equalassertions all passmake styleclean🤖 Generated with Claude Code
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com