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.

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

DevTools/Profiler: Replace DeprecatedFlyString with FlyString #25492

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ninadsachania
Copy link
Contributor

@ninadsachania ninadsachania commented Nov 27, 2024

This addresses part of #22447.

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Nov 27, 2024
@nico
Copy link
Contributor

nico commented Nov 27, 2024

Similar to what Sam said in discord and what's in #25498: I think most uses of DeprecatedFlyString are valid IIRC, so maybe we should just rename it to FlyBytes or something and keep it? @AtkinsSJ

@ninadsachania
Copy link
Contributor Author

Similar to what Sam said in discord and what's in #25498: I think most uses of DeprecatedFlyString are valid IIRC, so maybe we should just rename it to FlyBytes or something and keep it? @AtkinsSJ

What should be the new name? FlyByteString?

@nico
Copy link
Contributor

nico commented Nov 29, 2024

Similar to what Sam said in discord and what's in #25498: I think most uses of DeprecatedFlyString are valid IIRC, so maybe we should just rename it to FlyBytes or something and keep it? @AtkinsSJ

What should be the new name? FlyByteString?

That's a better name than FlyBytes, yes :) That sounds right to me.

@AtkinsSJ
Copy link
Member

It'd be worth checking what the users are. I'm still skeptical about it being useful to have a FlyByteString but I haven't looked into it deeply.

@nico
Copy link
Contributor

nico commented Nov 30, 2024

I think this is generally a good change. It's possible some of these should be removed, but at in many cases this is actually what we want.

However, it's a big patch and it will likely make cherry picks harder, so maybe it should wait until we're done with cherry picks (which will happen, when ladybird gets serious about swift, but also for other reasons eventually).

Or maybe this doesn't cause that many conflicts; we'll find out by just letting it sit for a while :)

Copy link

stale bot commented Dec 22, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@stale stale bot added the stale label Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 pr-needs-review PR needs review from a maintainer or community member stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants