Use insertion-ordered vector for LSQNumObj refined object list - #77
Closed
clemisch wants to merge 1 commit into
Closed
Use insertion-ordered vector for LSQNumObj refined object list#77clemisch wants to merge 1 commit into
clemisch wants to merge 1 commit into
Conversation
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.
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 |
Owner
|
Hi! I wanted to flag an alternative approach that addresses the root cause of #71 more completely. PR #78 introduces
The key difference from this PR:
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. |
18 tasks
Collaborator
Author
|
Not needed anymore after PR #78 |
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
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.
Replace
std::map<RefinableObj*>withstd::vector<pair<RefinableObj*>>inmvRefinedObjMap. The map iterated by pointer address (heap order), making the~deduplication suffixes appended to duplicate parameter names non-deterministic with respect toSetRefinedObj()call order.With the vector, suffix count follows registration order, so the first phase registered gets bare names (
V), the second getsV~, etc.Alleviates #71. Doesn't fix it, because parameter names are still changed in-place, just in a more deterministic way.