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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Core] Introducing proper insert method to PointerVectorSet #12087
[Core] Introducing proper insert method to PointerVectorSet #12087
Changes from 4 commits
e1702fd
0520260
abaeedd
4b34963
1a226e4
770d666
b3a9a76
b0ef86b
f5a7f05
8dfbe09
f2f3dcc
1c9af8e
d0ef0e7
3544a66
9c00f71
8b04cec
e86088b
5760f73
bc46807
6e7a68f
f604617
b1bc015
1437a25
8c58384
f84b135
b82885c
0555c33
4e5f741
2cfaa7b
ebea638
359f87e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a minor improvement, but I would not calling the insert here and just add the last statement of ifs in insert to avoid reevaluating for push_back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not possible, I need to add all if statements, because we need to do all the checks if the hint is discarded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here should be
else if (position_hint == cbegin())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change it :). As far as i see, current implementation is the same, except, it checks the value of the begin rather than the iterator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the value check should come in the main if like other statements of upper if elses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense and done :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As explained before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As explained before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we have an additional argument (or a separate method) for
insert_sorted_values
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is little bit dangerous. Thats why I put
insert(const PointVectorSet& rOther)
overload to avoid doing sorting and making it unique.insert(end(), value)
is havingO(N)
cost if we are inserting a sorted array which can be used to construct a quick sortedPointerVectorSet
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely not. The whole point of refactoring
PointerVectorSet
is to enforce its contract (sortedness and uniqueness) regardless of user input.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @sunethwarna that adding ordered set of entities is far faster than insert without assuming that. However creating the first PointerVectorSet would involve the sort.
Disagree with @matekelemen in the priority of robustness over performance for this case that the complexity of the algorithm changes from N to NlogN. Please note that in most of the mesh generation modelers we are adding millions of entities (to sub-modelparts) which we create with ordered indices (starting from the largest) and the first sort of those is an unnecessary operation
Nevertheless, I agree that this can be added afterward