Skip to content

Give FFPoint one way to copy itself - #191

Open
HugoFara wants to merge 1 commit into
devfrom
refactor/ffpoint-copy-assignment
Open

Give FFPoint one way to copy itself#191
HugoFara wants to merge 1 commit into
devfrom
refactor/ffpoint-copy-assignment

Conversation

@HugoFara

@HugoFara HugoFara commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

FFPoint is the type every coordinate in the simulation passes through, and copy is defined ambiguously with assignement.

FFPoint::FFPoint(const FFPoint& p) : x(p.x), y(p.y), z(p.z) { }   // misses it
// b = a                                                                                            // copies it

FFPoint b(a); and b = a; then produce different objects.

So the assignment operator is written out, directly below the copy-constructor, where a future member has to be added to both or the omission is visible in one file. FFVector has the identical shape and gets the identical treatment. Behaviour is unchanged: the member-wise copy written here is exactly what the compiler was generating.

Verification

Unit suite 4/4, runff KML and NetCDF both match within tolerance.

One thing noticed and deliberately left alone: nothing derives from either class, so those virtual destructors cost every FFPoint a vptr for nothing. Removing them is a future decision.

Contributes to #161.


This pull request, including its code changes and this description, was generated by Claude Opus 5, and reviewed manually before submitting.

Both hand-write a copy-constructor and leave assignment to the compiler,
so each class carries two independently maintained definitions of how to
copy itself. They agree today. They stop agreeing the moment anyone adds
a member: the generated assignment picks it up, the hand-written
constructor does not, leaving `FFPoint b(a)` and `b = a` producing
different objects. FFPoint carries every coordinate in the simulation.

Writing the assignment operator out puts both next to each other, so a
new member has to be added to both or the omission is visible in one
file. Behaviour is unchanged: three doubles each, owning nothing.

Contributes to #161, which is where this came to notice: 65 of its 428
warning lines were the missing operator. The issue attributes all 65 to
FFPoint; 51 are, the other 14 are FFVector, which has the same shape.

@antonio-leblanc antonio-leblanc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Went through both classes fully, not just the diff — FFPoint/FFVector only hold doubles, no owned resources, so this operator= is byte-for-byte what the compiler was already synthesizing implicitly.

Behavior's unchanged, and since nothing here touches ROS/propagation logic, no physics impact either — this is pure C++ hygiene, just silences -Wdeprecated-copy.

Safe to merge.

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