Skip to content

fix tevent::operator<#824

Draft
mercury233 wants to merge 1 commit into
Fluorohydride:masterfrom
mercury233:patch-copilot-1
Draft

fix tevent::operator<#824
mercury233 wants to merge 1 commit into
Fluorohydride:masterfrom
mercury233:patch-copilot-1

Conversation

@mercury233

Copy link
Copy Markdown
Collaborator

memcmp 2 structs may compare padding bytes, in edge cases which 2 structs are same it will cause UB.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR replaces tevent::operator<’s memcmp-based comparison (which can read padding bytes) with a field-by-field comparison to avoid undefined behavior when comparing otherwise-equal events.

Changes:

  • Removed #include <cstring> from field.cpp.
  • Reimplemented tevent::operator< as a lexicographic comparison across tevent members instead of memcmp.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread field.cpp
Comment on lines +50 to +52
if(trigger_card != v.trigger_card) return trigger_card < v.trigger_card;
if(event_cards != v.event_cards) return event_cards < v.event_cards;
if(reason_effect != v.reason_effect) return reason_effect < v.reason_effect;

Copilot AI Mar 7, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operator< now compares raw pointers with <. In C++, relational comparison of unrelated pointers is unspecified and can violate the strict-weak-ordering requirement for std::set/std::map keys. Use std::less<card*>{}(trigger_card, v.trigger_card) (and similarly for group* / effect*) to get a well-defined total order; also consider custom comparators for other ordered containers that compare effect* directly (e.g., std::set<std::pair<effect*, tevent>>).

Copilot uses AI. Check for mistakes.
@mercury233 mercury233 marked this pull request as draft March 7, 2026 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants