Baked data: use VarULE to store data when specified #6133

sffc · 2025-02-16T00:55:58Z

I did it by creating a trait, MaybeAsVarULE, which is implemented on data structs, similar to how Bake and Serialize are implemented on data structs.

The trait has an associated type that I can leverage in the baked exporter without having to do dyn Any downcasting.

I tried to keep my understanding of @robertbastian's position in mind while writing this PR (don't add anything hyper-specific to bake in the provider crate), and I think I achieved this, since the thing I added is on the same level as the existing format-specific traits.

See the hello world data for an example. I didn't apply it to the other data markers yet.

Manishearth

I think this design of sticking it on the export traits is good: it avoids cluttering core data marker infra whilst still providing extra metadata for export.

Manishearth · 2025-02-16T10:44:26Z

provider/core/src/marker.rs

+    fn maybe_as_varule(&self) -> Option<&Self::VarULE>;
+}
+
+/// Implements [`MaybeAsVarULE`] on a type that is NOT representable


thought: the reverse default might be better UX for ICU4X devs so they do not accidentally feel forced to structure all of their data structs to support VarULE. I know Younies has found this confusing before.

but I figure there's a reason behind this choice here. if it's just macroability I think the macro can be done differently.

the reverse default

I don't understand what you mean. Do you mean the name of the trait? The name of the macro? Something else?

This macro opts out of this system by default.

I'd rather have the data marker macro opt in by default, with a flag to opt out

I see, so this is primarily feedback about what the macro is named and what flags it takes?

Not the name, just the default behavior, yes.

Basically: if I define a new data struct, the default behavior of the data struct/marker macros should not make me think I need to impl VarULE. The non-None impl of this trait should be opt in.

The macro does what I think you want it to do? It implements the trait to return None. If you want not None, there is no macro; implement the trait manually.

Ah, no, I'm talking about the data_marker/data_struct macros. And I see the source of my confusion: The data_struct macro does handle this correctly, however in list/properties we have data structs that do not use the macro (why?) and they need to explicitly call it.

I saw explicit calls to this macro and thought the trait was opt-out.

I applied @robertbastian's suggestion to create a new icu_provider::marker::data_struct! macro rules.

utils/zerovec/src/ule/plain.rs

provider/core/src/marker.rs

sffc · 2025-02-16T14:09:19Z

Created #6135 to reduce the diff

Toward #5230 Reduces the diff in #6133 Not intended to be controversial. Autosubmit = true

robertbastian · 2025-02-17T17:41:26Z

provider/core/src/hello_world.rs

@@ -41,6 +42,21 @@ impl Default for HelloWorld<'_> {
    }
 }

+impl MaybeAsVarULE for HelloWorld<'_> {


Issue: I think allowing implementation of this trait on arbitrary data structs is going to be super confusing (and it should definitely not use From to do the reverse).

HelloWorldV1::DataStruct should just be Cow<'static, str>, and this trait should be implemented for Cow<'static, str> in icu_provider.

I'm okay implementing it on Cow, but we still need the no-op version of the trait implement on other things. I don't see a reason to not allow it to be implemented on arbitrary data structs.

(I am still allowing the trait to be implemented on any data struct)

robertbastian · 2025-02-17T17:42:56Z

components/list/src/provider/mod.rs

@@ -69,6 +69,8 @@ data_marker!(
    ListFormatterPatterns<'static>,
 );

+icu_provider::marker::does_not_deref_to_varule!(ListFormatterPatterns<'_>);


issue: this is an opt-out, but it should be opt-in

Without specialization, I need to implement the trait on all data structs, and use runtime Option to determine if it is actually implement or not.

I don't see how specialization is relevant for opt out vs opt in? We're using a macro, the macro can default to generating a blank impl, and one can ask the macro to not do so.

This is my comment above about the reverse default.

I don't see how specialization is relevant for opt out vs opt in

Ideally I would have a blanket impl that returns None and then a small number of specific impls that return Some. Instead, everyone needs to decide how to implement the trait, which I think @robertbastian doesn't like.

My objection at least is that people need to explicitly opt out: it's not an objection about the trait architecture, it's an objection about the macro architecture.

How about we just make this

Suggested change

icu_provider::marker::does_not_deref_to_varule!(ListFormatterPatterns<'_>);

icu_provider::data_struct!(ListFormatterPatterns<'_>);

Then we can use something like

icu_provider::data_struct!(HelloWorld<'_>, encoding: |v| v.message.as_str(), |e| Self { message: Cow::Borrowed(v) });

to do the optimization.

I did something like this.

robertbastian · 2025-02-17T17:43:48Z

provider/core/macros/src/lib.rs

if we did the marker name cleanup first, we didn't have to keep maintaining this macro

To the contrary, I want to land this before marker cleanup so that we can make a decision on a case-by-case basis on whether to apply the optimization.

Applying this optimization is not 2.0 blocking. This is scope creep.

Applying this optimization is not 2.0 blocking. This is scope creep.

I consider it 2.0 to have a design for this. This is a specific design proposal. It involves changing the bounds of ExportableDataPayload, which seems like something that would be smoother to do in a major release.

sffc · 2025-02-17T20:00:47Z

This PR is passing CI and ready from my perspective. I am unclear whether the open comments are bikeshed issues or fundamental design issues, and how you hope that they be resolved.

Manishearth · 2025-02-17T22:02:53Z

@sffc From my side, the trait naming is a bikeshed that can be deferred, and the macro opt-in/opt-out is a concern I'd prefer solved here but is not a major design issue.

The overall design looks good.

Manishearth

OK to land from my end, would prefer the trait be renamed, but can happen later.

robertbastian · 2025-02-18T14:15:10Z

provider/core/src/marker.rs

+///
+/// For most data structs, a default implementation available via
+/// [`does_not_deref_to_varule`] can be used.
+pub trait MaybeAsVarULE {


I think this trait can be behind the export feature

Discussion with @robertbastian:

The maybe_as_var_ule function can be gated, but not the whole trait, because we need the associated type

Add from_varule to replace the From bound

Use a never type (empty enum) for the non-implemented case; this lets the from_varule match on it (and be implementable)

Not 100% sure how that gating would work, but will have a look when you have it ready. Would rather not have implementors of this forced to enable the export feature, i think

(yes, that might mean not gating it, that's fine by me)

but no strong opinion, let's see how the gating works

Before I do the feature-gating, to make sure we is on the same page, here are some facts

The VarULE associated type is used at runtime in data structs that apply the optimization

The maybe_as_varule function is only used during data export

Most component crates have a "datagen" feature; maybe we can assume it is all component crates

So if we want to move forward with the gating, there are a few choices

Feature-gate the function in the trait and the impls. In icu_provider, it is in the "export" feature, and in the impls, it is in the "datagen" feature of the specific crates.

Do not feature-gate the trait in icu_provider. Feature-gate the impls except when the impl is the non-default impl, since that is the case when it is used at runtime.

Both 1 and 2.

I think I prefer either option 0 (don't feature-gate) or option 2 (feature-gate the whole trait).

I prefer not feature gating. This is a trait impl and I'd rather keep it clean.

I'm happy to have the impl itself gated on datagen: the Encode impls could pull in additional deps, but I think the trait itself should always be available to avoid specifying transitive deps.

the Encode impls could pull in additional deps

This is compelling to me.

I would prefer not to "split" the trait with a function enabled only on certain features enabled. That sounds like a bit of a disaster since the features need to be always 100% in sync. If the icu_provider trait enables its export feature, but a crate doesn't have its datagen feature enabled, then you break code.

So I would like to keep the trait unconditionally exported from icu_provider and conditionally implemented, in whole, in component crates.

I'm aligned

robertbastian · 2025-02-18T14:16:59Z

components/list/src/provider/mod.rs

@@ -69,6 +69,8 @@ data_marker!(
    ListFormatterPatterns<'static>,
 );

+icu_provider::marker::does_not_deref_to_varule!(ListFormatterPatterns<'_>);


How about we just make this

Suggested change

icu_provider::marker::does_not_deref_to_varule!(ListFormatterPatterns<'_>);

icu_provider::data_struct!(ListFormatterPatterns<'_>);

Then we can use something like

icu_provider::data_struct!(HelloWorld<'_>, encoding: |v| v.message.as_str(), |e| Self { message: Cow::Borrowed(v) });

to do the optimization.

robertbastian · 2025-02-18T14:19:31Z

provider/baked/src/zerotrie.rs

+use zerovec::VarZeroVec;
+
+#[cfg(feature = "export")]
+pub(crate) enum BakedValue<'a> {


observation: this optimisation should also be applied to the binary search data store, or we should delete it.

Is it ok if I make an issue to fix the binary search baked backend instead of deleting or updating it in this PR?

robertbastian · 2025-02-18T14:21:23Z

provider/baked/src/zerotrie.rs

+        get_index(self.trie, id, attributes_prefix_match)
+            // Safety: Allowed since `i` came from the trie and the field safety invariant
+            .map(|i| unsafe { self.values.get_unchecked(i) })
+            .map(From::from)


issue: please introduce a specific trait for this instead of abusing From.

I pushed a possible design. I don't think I like it.

As discussed in DM, we need 2 different associated types with this design: one that is a reference and one that is a VarULE. The one that is a reference, which I named EncodeAsVarULE,

I disagree that my use of From is an "abuse" of the trait, but I acknowledge the point of view that From looks like it might do some sort of transformation on the string. I believe that ZeroFrom does not carry similar baggage. I would prefer to revert to something closer to what I had before, with a single associated type, but using ZeroFrom.

^ would like to highlight this open question

Given the proposed conclusion on #6133 (comment), I want to double-down on my position that we should use two traits, one for datagen and one for runtime. I would like the datagen trait to be the MaybeExportAsVarULE and the runtime trait to be ZeroFrom. I am willing to use custom traits in both situations.

Technically this could be 3 traits, since there are 3 unique, disjoint things we need, in different situations:

maybe_as_varule -> Option<(impl VarULE or impl EncodeAsVarULE)>

Needed in datagen only

pub type VarULE

Needed in datagen for all structs and at runtime for structs that use it

from_varule -> Self

Needed at runtime for structs that use it

A potential design for this would be 3 traits: (2) would be a base trait and (1) and (3) would extend it.

Using ZeroFrom as the runtime trait sounds good to me.

Currently exploring this constellation of traits:

pub trait MaybeAsVarULE { type VarULE: ?Sized + VarULE; } #[cfg(feature = "export")] pub trait MaybeEncodeAsVarULE: MaybeAsVarULE { fn maybe_encode_as_varule(&self) -> Box<Self::VarULE>; } pub trait FromVarULE<'a>: MaybeAsVarULE { fn from_varule(varule: &'a Self::VarULE) -> Self; }

What's the deficiency in using ZeroFrom? We have tons of magical derive infra for that, and none for a new trait.

The trait makes it more explicit that it is the reverse operation as MaybeEncodeAsVarULE

A hand-written impl is sometimes required since there may be intermediate fields involved; for example, if the data struct is Cow<str>, the auto-derived impl will work, but not if the data struct is HelloWorld(Cow<str>)

@robertbastian was opposed to using ZeroFrom

(none of these points are super compelling to me and I am happy to switch to ZeroFrom)

robertbastian · 2025-02-18T19:54:31Z

components/list/src/provider/mod.rs

@@ -69,7 +69,7 @@ data_marker!(
    ListFormatterPatterns<'static>,
 );

-icu_provider::marker::does_not_deref_to_varule!(ListFormatterPatterns<'_>);
+icu_provider::marker::data_struct!(ListFormatterPatterns<'_>);


this should not be in the marker module

I can export this from icu_provider. Do you also have an opinion on where the trait and type (MaybeExportAsVarULE, NeverVarULE) should be exported?

robertbastian · 2025-02-18T19:56:36Z

provider/core/src/marker.rs

+            }
+        }
+    };
+    ($ty:path, varule: $varule:path, as_varule: $as_varule:path, from_varule: $from_varule:path) => {


Suggested change

($ty:path, varule: $varule:path, as_varule: $as_varule:path, from_varule: $from_varule:path) => {

($ty:ty, varule: $varule:ty, as_varule: $as_varule:expr, from_varule: $from_varule:expr) => {

robertbastian · 2025-02-18T20:00:58Z

provider/core/src/hello_world.rs

+impl<'a> HelloWorld<'a> {
+    fn as_varule(&'a self) -> &'a str {
+        &self.message
+    }
+    fn from_varule(message: &'a str) -> Self {
+        Self {
+            message: Cow::Borrowed(message)
+        }
+    }
+}
+
+crate::marker::data_struct!(
+    HelloWorld<'a>,
+    varule: str,
+    as_varule: HelloWorld::as_varule,
+    from_varule: HelloWorld::from_varule
+);


Suggested change

impl<'a> HelloWorld<'a> {

fn as_varule(&'a self) -> &'a str {

&self.message

}

fn from_varule(message: &'a str) -> Self {

Self {

message: Cow::Borrowed(message)

}

}

}

crate::marker::data_struct!(

HelloWorld<'a>,

varule: str,

as_varule: HelloWorld::as_varule,

from_varule: HelloWorld::from_varule

);

crate::marker::data_struct!(

HelloWorld<'a>,

varule: str,

as_varule: |v| &v.message,

from_varule: |v| Self { message: Cow::Borrowed(v) }

);

robertbastian · 2025-02-18T20:02:42Z

provider/core/src/export/payload.rs

-    for<'a> <M::DataStruct as Yokeable<'a>>::Output: Bake + BakeSize + serde::Serialize + PartialEq,
+    for<'a> <M::DataStruct as Yokeable<'a>>::Output:
+        Bake + BakeSize + serde::Serialize + MaybeExportAsVarULE<'a> + PartialEq,
+    for<'a> <<M::DataStruct as Yokeable<'a>>::Output as MaybeExportAsVarULE<'a>>::VarULE: VarULE,


I'm really not a fan of naming associated types by their bound. ...::VarULE: VarULE does not really tell me what any of this is. Can you come up with clear names for these associated type, like we renamed Yokeable to DataStruct?

It tells you what it literally is (it is an associated VarULE type, which is 100% correct), just not what it does.

I don't have any other name that is as precise and correct as this. But if you want to brainstorm, and if you like the DataStruct themed naming:

DataStructAsVarULE

OptionalVarULE

CompactDataStruct

DataStruct (it is on a different trait so using the same name is possible)

robertbastian · 2025-02-18T20:03:28Z

provider/baked/src/zerotrie.rs

+    M::DataStruct: MaybeExportAsVarULE<'static>,
+    <M::DataStruct as MaybeExportAsVarULE<'static>>::VarULE: VarULE,
+    <M::DataStruct as MaybeExportAsVarULE<'static>>::EncodeAsVarULE:
+        From<&'static <M::DataStruct as MaybeExportAsVarULE<'static>>::VarULE>,


why is there still a From bound?

I need to transform &str (a &Self::VarULE) to &str (a Self::EncodeAsVarULE)

Manishearth · 2025-02-18T21:20:00Z

provider/core/macros/src/lib.rs

@@ -359,6 +365,11 @@ fn data_struct_impl(attr: DataStructArgs, input: DeriveInput) -> TokenStream2 {
        }
    }

+    // TODO: Allow this to be configured


nit: update comment to "TODO Allow the automatic impl of MaybeAsVarULE to be configured" since it's not obvious from the macro anymore

Manishearth · 2025-02-18T21:21:09Z

provider/core/src/export/payload.rs

-    for<'a> <M::DataStruct as Yokeable<'a>>::Output: Bake + BakeSize + serde::Serialize + PartialEq,
+    for<'a> <M::DataStruct as Yokeable<'a>>::Output:
+        Bake + BakeSize + serde::Serialize + MaybeExportAsVarULE<'a> + PartialEq,
+    for<'a> <<M::DataStruct as Yokeable<'a>>::Output as MaybeExportAsVarULE<'a>>::VarULE: VarULE,


Manishearth · 2025-02-18T21:22:08Z

provider/core/src/marker.rs

+    /// if it cannot be represented as [`VarULE`].
+    ///
+    /// This is expected to be `&'a Self::VarULE` for compatible types.
+    type EncodeAsVarULE: 'a;


thought: can the complicated Encode bound be moved here?

Manishearth · 2025-02-18T21:22:56Z

provider/core/src/marker.rs

+    };
+}
+#[doc(inline)]
+pub use __data_struct as data_struct;


thought: I'm assuming that the intent is for this macro to over time take over all purposes of the old proc macro?

no, most of what the old proc macro does is now done by data_marker!

sffc · 2025-02-18T22:15:44Z

provider/core/src/marker.rs

+/// Safety checklist for VarULE:
+///
+/// 1. No uninitialized or padding bytes (empty type)
+/// 2. Type is an empty enum
+/// 3. `VarULE::validate_bytes()` always returns errors
+/// 4. `VarULE::validate_bytes()` always returns errors
+/// 5. `VarULE::from_bytes_unchecked()` returns the same pointer
+/// 6. All other methods are left with their default impl
+/// 7. Byte equality is semantic equality
+unsafe impl VarULE for NeverVarULE {
+    fn validate_bytes(_bytes: &[u8]) -> Result<(), UleError> {
+        Err(UleError::parse::<Self>())
+    }
+    unsafe fn from_bytes_unchecked(bytes: &[u8]) -> &Self {
+        // Note: this returns a thin pointer instead of a fat pointer
+        &*(bytes.as_ptr() as *const Self)
+    }
+}


@Manishearth Flagging this code for a more detailed unsafe review.

This shouldn't be an empty enum IMO, there are far too many tricky things there. Let's just use a ZST and have it always error.

The empty enum is required in order to implement @robertbastian's preferred solution (discussed in the meeting this morning) where from_varule is always implemented; the empty enum means that types that don't support VarULE can match on it. However, I would prefer a different set of traits.

Are you concerned about from_bytes_unchecked taking in a fat pointer and returning a thin pointer?

The unsafe review for zerovec becomes much trickier when you start considering uninhabited types, since it's insta-UB to have an Empty, &Empty, or other such types floating around. We have not reviewed the zerovec crate for this, and I am unwilling to perform such a deep review and updation of invariants for this change.

Generally uninhabited types do not mix well with unsafe code. It's typical to assume they won't be introduced around this type of unsafe code when reviewing it, and opting uninhabited types into unsafe traits is a major red flag that requires reevaluation of this assumption.

validate_bytes always failing makes this plausibly correct, but I'm not able to quickly verify that in a nonglobal way. We have a lot of code using these traits.

IMO just abort instead of matching on the empty enum.

Yes, I will remove the condition and impl for uninhabited types.

Are you concerned about from_bytes_unchecked taking in a fat pointer and returning a thin pointer?

^ can you address this?

To clarify: I can do either of the following

Implement ULE on a ZST like (), and use [()] as the placeholder VarULE type (this is what I did in an earlier version of this PR)

Implement VarULE on the ZST and use () directly as the placeholder VarULE, but then it is a thin pointer.

Oh, I hadn't noticed that had changed as well. I am somewhat concerned about that, we do assume VarULE types are DSTs but I don't actually think we document it as a requirement on the trait, or have unsafe code relying on it.

So it should be fine. But the [ZST] approach is also good.

sffc · 2025-02-18T22:15:50Z

utils/zerovec/src/ule/mod.rs

@@ -261,7 +261,7 @@ where
 /// Safety checklist for `VarULE`:
 ///
 /// 1. The type *must not* include any uninitialized or padding bytes.
-/// 2. The type must have an alignment of 1 byte.
+/// 2. The type must have an alignment of 1 byte, or it is a ZST or an empty enum.


@Manishearth Flagging this code for a more detailed unsafe review.

let's exclude empty enum

"or it is a ZST that is safe to construct" so that we disallow free construction of token ZSTs

sffc added 7 commits February 15, 2025 22:16

impl ULE for ()

522e1ab

Add MaybeAsVarULE and impl on all data structs

309d12d

Start integrating into BakedExporter

8d45d49

Implement on HelloWorld

5c34469

datagen

a814e3b

Almost done

781fe51

Add the missing impl

1eee363

sffc requested review from Manishearth, robertbastian and a team as code owners February 16, 2025 00:55

Manishearth reviewed Feb 16, 2025

View reviewed changes

sffc added 4 commits February 16, 2025 14:35

fmt

9b18826

Reduce diff; DRY

5bd8b18

clean datagen (not word break?)

255122a

datagen for word break (it appears the same, it just doesn't format)

1eb0897

sffc mentioned this pull request Feb 16, 2025

Baked: Change DataStore to return DataPayload #6135

Merged

sffc added 3 commits February 16, 2025 15:24

features

c1e86b1

macro test

6eea0dd

clippy

fc564ea

robertbastian pushed a commit that referenced this pull request Feb 17, 2025

Baked: Change DataStore to return DataPayload (#6135)

d8d0e6b

Toward #5230 Reduces the diff in #6133 Not intended to be controversial. Autosubmit = true

robertbastian reviewed Feb 17, 2025

View reviewed changes

sffc requested a review from robertbastian February 17, 2025 18:02

Merge branch 'main' into maybe-as-varule

e73da50

sffc requested a review from Manishearth February 17, 2025 19:59

Manishearth previously approved these changes Feb 18, 2025

View reviewed changes

robertbastian requested changes Feb 18, 2025

View reviewed changes

MaybeExportAsVarULE

81035bb

sffc added 6 commits February 18, 2025 20:42

NeverVarULE

4cc5166

remove impl ULE for ()

2e09206

Add MaybeExportAsVarULE::from_varule

035abb3

Rename macro to data_struct

8c12687

Docs for the struct

d6dcd09

Use macro to implement the trait for HelloWorld

8ffb284

sffc dismissed Manishearth’s stale review via 8ffb284 February 18, 2025 19:42

fmt

889a44f

sffc requested review from robertbastian and Manishearth February 18, 2025 20:01

robertbastian reviewed Feb 18, 2025

View reviewed changes

Manishearth reviewed Feb 18, 2025

View reviewed changes

sffc commented Feb 18, 2025

View reviewed changes

sffc requested a review from Manishearth February 18, 2025 22:16

New three traits; move to icu_provider::ule module

c3e539f

sffc mentioned this pull request Feb 19, 2025

Baked data is big, and compiles slowly, for finely sliced data markers #5230

Open

Switch from box to reference for now (unicode-org#5230 (comment))

8eb18c5

	icu_provider::marker::does_not_deref_to_varule!(ListFormatterPatterns<'_>);
	icu_provider::data_struct!(ListFormatterPatterns<'_>);

	($ty:path, varule: $varule:path, as_varule: $as_varule:path, from_varule: $from_varule:path) => {
	($ty:ty, varule: $varule:ty, as_varule: $as_varule:expr, from_varule: $from_varule:expr) => {

Baked data: use VarULE to store data when specified #6133

Are you sure you want to change the base?

Baked data: use VarULE to store data when specified #6133

Conversation

sffc commented Feb 16, 2025 • edited Loading

Manishearth left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Feb 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Feb 17, 2025

Manishearth commented Feb 17, 2025

Manishearth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Feb 16, 2025 •

edited

Loading

Manishearth left a comment •

edited

Loading

sffc Feb 17, 2025 •

edited

Loading

sffc Feb 19, 2025 •

edited

Loading

Manishearth Feb 19, 2025 •

edited

Loading