Skip to content

Use insertion-ordered vector for LSQNumObj refined object list - #77

Closed
clemisch wants to merge 1 commit into
vincefn:masterfrom
clemisch:LSQ-ordered
Closed

Use insertion-ordered vector for LSQNumObj refined object list#77
clemisch wants to merge 1 commit into
vincefn:masterfrom
clemisch:LSQ-ordered

Conversation

@clemisch

@clemisch clemisch commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Replace std::map<RefinableObj*> with std::vector<pair<RefinableObj*>> in mvRefinedObjMap. The map iterated by pointer address (heap order), making the ~ deduplication suffixes appended to duplicate parameter names non-deterministic with respect to SetRefinedObj() call order.

With the vector, suffix count follows registration order, so the first phase registered gets bare names (V), the second gets V~, etc.

Alleviates #71. Doesn't fix it, because parameter names are still changed in-place, just in a more deterministic way.

Replace std::map<RefinableObj*> with std::vector<pair<RefinableObj*>>
in mvRefinedObjMap. The map iterated by pointer address (heap order),
making the '~' deduplication suffixes appended to duplicate parameter
names non-deterministic with respect to SetRefinedObj() call order.

With the vector, suffix count follows registration order, so the first
phase registered gets bare names ('V'), the second gets 'V~', etc.
@vincefn

vincefn commented Jul 22, 2026

Copy link
Copy Markdown
Owner

I'm still not comfortable with that: the underlying issue is that the parameters are renamed in a weird and unpredictable way, and this is more a workaround than a good solution. Followup in #71

@vincefn

vincefn commented Jul 23, 2026

Copy link
Copy Markdown
Owner

Hi! I wanted to flag an alternative approach that addresses the root cause of #71 more completely.

PR #78 introduces RefinableParProxy, a lightweight subclass of RefinablePar that:

  • Wraps the original parameter and keeps a local hierarchy-prefixed name (ParentClass:ParentName:paramName)
  • Delegates all non-name operations (value, limits, flags, type) to the original, so changes propagate correctly
  • Avoids mutating original parameter names at all (not just making the order deterministic)

The key difference from this PR:

PR #77 (this PR) PR #78 (alternative)
Original names mutated? Yes (deterministically) No
Attribute changes propagate? N/A (names are on originals) Yes (delegation)
Scope of change LSQNumObj map ordering RefinableObj core + new class
Fixes #71 completely? Partially (order is stable, but names still change) Yes

Since the two PRs target the same issue, they should probably not both be merged. Happy to discuss the trade-offs — the proxy approach is more invasive but eliminates the mutation entirely.

@clemisch

Copy link
Copy Markdown
Collaborator Author

Not needed anymore after PR #78

@clemisch clemisch closed this Aug 24, 2026
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