-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Deprecate Resize() from pdata slice APIs #3573
Deprecate Resize() from pdata slice APIs #3573
Conversation
Signed-off-by: Anthony J Mirabella <[email protected]>
…tor into feat/pdata/remove_resize
* Removed Resize() * Added EnsureCapacity() and AppendEmptyN() Signed-off-by: Anthony J Mirabella <[email protected]>
Signed-off-by: Anthony J Mirabella <[email protected]>
Signed-off-by: Anthony J Mirabella <[email protected]>
Signed-off-by: Anthony J Mirabella <[email protected]>
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.
Most of the changes are fine, but replacement of Resize by RemoveIf is not an improvement. I am not sure this is net positive change. Perhaps only keep changes to AppendEmptyN (which are reasonable) but also keep Resize for the cases when we need to make the slice smaller?
dest.DoubleGauge().DataPoints().RemoveIf(func(_ pdata.DoubleDataPoint) bool { | ||
return filterDestPoints() | ||
}) |
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.
This looks pretty awkward and harder to understand. Resize
seemed to be not a bad idea?
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.
Yeah, and I think this was the simpler one to understand. It would be sad to have to keep Resize
around just for this, but I'm not sure there is a better option.
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 rewrote this to avoid copying the entire metric when its datapoints are to be split. I think this was the only place where using Resize()
would still be attractive from a code reduction standpoint, but this implementation is probably still preferable.
…tor into feat/pdata/remove_resize
Signed-off-by: Anthony J Mirabella <[email protected]>
I would say that we should apply the rule of minimal public API if possible, I do understand that these changes are not clear, but I would also say that right now we do something strange, we "copy" the entire metric then remove extra points, probably we should do the opposite and copy only the points we need to copy, so then no need to re-size. @Aneurysm9 I have a question for you because I feel we can move even further and remove "AppendEmptyN" as well. What do you think about that as our initial API? I know it will add some extra overhead, but will allow us to think better about the API we really need and play with different possibilities? My 2 cents when it comes to pdata API is if we can keep it as small as possible we will be able to do more optimizations later, otherwise we may be blocked by the public APIs we expose right now. |
I'll take another pass at this. I had saved the split functions for last since they were clearly different and I couldn't really think of a better way to handle them at the time. Maybe tomorrow I'll see it with fresh eyes.
Definitely agree that minimizing the API is a good goal and |
…tor into feat/pdata/remove_resize
@Aneurysm9 any update on this? |
Signed-off-by: Anthony J Mirabella <[email protected]>
This is my current task. |
…unless necessary Signed-off-by: Anthony J Mirabella <[email protected]>
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.
@Aneurysm9 in order to be able to merge this (since we are close to the release date, and avoid waiting until the release), I would suggest to keep "Resize" as deprecated in this PR - only the function we can remove the tests. This way contrib will not break and we can do the contrib upgrade before or after the release.
What do you think?
That seems reasonable. I was having trouble getting contrib to build with rewrites for core, so I'm not sure how much will actually be impacted and taking a two-step approach to removing Resize is probably safest. |
…tor into feat/pdata/remove_resize
Signed-off-by: Anthony J Mirabella <[email protected]>
Please rebase :) sorry |
@Aneurysm9 I gave a try to resolve conflicts from the UI, let's see what i've done :)) |
@bogdandrutu I think the contrib tests are having a struggle because |
Description: Removes
Resize()
from the pdata slice APIs, replacing it withEnsureCapacity()
andAppendEmptyN()
methods. Many existing uses ofResize()
can be directly replaced withAppendEmptyN()
(which in turn callsEnsureCapacity()
) to allocate new, empty slice members. Other uses are replaced with a single call toEnsureCapacity()
followed by successive calls toAppendEmpty()
, primarily when an upper bound on the number of slice elements is known but fewer may be used in the end. This avoids a second call toResize()
that had been necessary to remove the empty elements at the end of the list.Link to tracking Issue: #2488
Testing: Existing tests were updated to reflect the new API.