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

AK: Cherry Pick the Optional changes from LB #25337

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Hendiadyoin1
Copy link
Contributor

This cherry picks LadybirdBrowser/ladybird#2032
With additional changes to accommodate #22870


This also adds a compat commit introducing ASSERT as an alias to VERIFY
@nico tell me if this is correct

@Hendiadyoin1 Hendiadyoin1 marked this pull request as draft November 9, 2024 15:24
@github-actions github-actions bot added 👀 pr-needs-review PR needs review from a maintainer or community member and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels Nov 9, 2024
@Hendiadyoin1
Copy link
Contributor Author

Ah dammit it depends on some random other commit introducing new String functions,
investigating

trflynn89 and others added 9 commits November 9, 2024 16:27
This feature is unused in Ladybird and will complicate an upcoming patch
to hand-off StringBuilder's memory to String.

(cherry picked from commit af220af8bf15f9eb297a165e6d2ae3bf5bbc49fc)
Opening StringData.h in any clangd-enabled editor previously resulted in
a sea of clangd errors.

(cherry picked from commit 77eef8a8f6723b1e2fb5107aac7cf39bc1b40dad)
Currently, invoking StringBuilder::to_string will re-allocate the string
data to construct the String. This is wasteful both in terms of memory
and speed.

The goal here is to simply hand the string buffer over to String, and
let String take ownership of that buffer. To do this, StringBuilder must
have the same memory layout as Detail::StringData. This layout is just
the members of the StringData class followed by the string itself.

So when a StringBuilder is created, we reserve sizeof(StringData) bytes
at the front of the buffer. StringData can then construct itself into
the buffer with placement new.

Things to note:
* StringData must now be aware of the actual capacity of its buffer, as
  that can be larger than the string size.
* We must take care not to pass ownership of inlined string buffers, as
  these live on the stack.

(cherry picked from commit 29879a69a4b2eda4f0315027cb1e86964d333221;
amended minor conflict in AK/String.h due to us not having
String::from_utf16() from LadybirdBrowser/ladybird#674, last commit)
For performance, rather than slowly incrementing the capacity of the
rope string's buffer, compute an approximate length for that buffer to
be reserved up front.

(cherry picked from commit e8f4ae487d228dac491a446ed548400176331ae9)
In contrast to Ladybird, we won't disable assertions in release builds,
though.
Using CRTP and `static_cast`s because "deducing this" is
still not fully supported yet.

(cherry picked from commit a70ed6a2ada94df63e12c94d35166b7d4ca0b90a)
`Optional` and `Variant` both use essentially the same pattern of only
declaring a copy constructor/move constructor/destructor and copy/move
assignment operator if all of their template parameters have one.

Let's move these into a macro to avoid code duplication and to give a
name to the thing we are trying to accomplish.

(cherry picked from commit fcdf3014f1bfbc9b90560d0399f0dbc33ccca9b6)
Slice the size of `Optional<{,Fly}String>` in half by introducing
`UINTPTR_MAX` as an invalid bit pattern for these values.

(cherry picked from commit 2457118024a9d3b39178bba0e92c34923b381f20)
This was previously still valid since the `Optional<String>` move
constructor copied its input, but since the move is now destructive,
the code no longer works.

(cherry picked from commit a733e2a31c0c67ee262a1676c5ad8e0d7415dbbb)
@Hendiadyoin1
Copy link
Contributor Author

Depends on: #25131

@nico
Copy link
Contributor

nico commented Nov 9, 2024

(That one also depends on other things.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants