Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use extend() with iterator as appropriate in normalizer and collator #2482

Closed
hsivonen opened this issue Aug 30, 2022 · 12 comments · Fixed by #2861
Closed

Use extend() with iterator as appropriate in normalizer and collator #2482

hsivonen opened this issue Aug 30, 2022 · 12 comments · Fixed by #2861
Labels
C-collator Component: Collation, normalization good first issue Good for newcomers help wanted Issue needs an assignee S-tiny Size: Less than an hour (trivial fixes)

Comments

@hsivonen
Copy link
Member

Instead of pushing in a loop, use extend.
@hsivonen hsivonen added S-tiny Size: Less than an hour (trivial fixes) C-collator Component: Collation, normalization labels Aug 30, 2022
@hsivonen hsivonen mentioned this issue Aug 30, 2022
Merged
@robertbastian robertbastian added the good first issue Good for newcomers label Aug 30, 2022
@kelebra
Copy link
Contributor

kelebra commented Sep 8, 2022 •
edited
Loading

I would like to try and tackle this issue. Two questions if you don't mind me asking:

  • should I introduce new benchmark for the functionality to measure if the suggestion helped?
  • I found another instance of mentioned code pattern (for ... vec.push) in the same file, should I replace it too?

@kelebra kelebra mentioned this issue Sep 8, 2022
Merged
@robertbastian
Copy link
Member

There are a few instances across those two crates, they should all be updated. I don't think benchmarks are necessary.

@sffc
Copy link
Member

sffc commented Sep 8, 2022 •
edited
Loading

Does #2531 fix this issue?

@robertbastian
Copy link
Member

I'm pretty sure there were more than 2 instances

@kelebra
Copy link
Contributor

kelebra commented Sep 8, 2022

I can handle remaining instances later today 👍

@kelebra kelebra mentioned this issue Sep 9, 2022
Merged
@sffc
Copy link
Member

sffc commented Oct 17, 2022

@hsivonen Can you set an assignee (or "help wanted") and a milestone (or "backlog")?

@sffc
Copy link
Member

sffc commented Dec 1, 2022

@hsivonen @kelebra Is this issue fixed?

@kelebra
Copy link
Contributor

kelebra commented Dec 1, 2022

I think I addressed all the remaining cases in the PR before, so should be fixed.

@hsivonen
Copy link
Member Author

hsivonen commented Dec 2, 2022

One instance left still.

@hsivonen hsivonen added help wanted Issue needs an assignee backlog labels Dec 2, 2022
@kelebra
Copy link
Contributor

kelebra commented Dec 2, 2022

I can take care of that, would you mind assigning this to me?

@sffc
Copy link
Member

sffc commented Dec 2, 2022

@kelebra I can't assign the issue since you're not currently in the org, but feel free to contribute a PR!

@kelebra kelebra mentioned this issue Dec 2, 2022
Merged
@sffc
Copy link
Member

sffc commented Dec 5, 2022

I take that this is fixed now; thanks, @kelebra!

@sffc sffc closed this as completed Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-collator Component: Collation, normalization good first issue Good for newcomers help wanted Issue needs an assignee S-tiny Size: Less than an hour (trivial fixes)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Address last for/vec.push case in favor of vec.extend kelebra/icu4x
4 participants
@hsivonen @sffc @kelebra @robertbastian