Skip to content

core: Simplify & support non-ident common strings#23479

Open
kjarosh wants to merge 3 commits intoruffle-rs:masterfrom
kjarosh:common-strings
Open

core: Simplify & support non-ident common strings#23479
kjarosh wants to merge 3 commits intoruffle-rs:masterfrom
kjarosh:common-strings

Conversation

@kjarosh
Copy link
Copy Markdown
Member

@kjarosh kjarosh commented Apr 17, 2026

This PR replaces the define_common_strings! macro-rules with a procedural macro that automatically generates field names from the provided byte string literals. This reduces duplication in common.rs by removing the need to manually specify str_ prefixes for every common string.

After refactoring define_common_strings! to a proc macro, we can escape non-ident characters in strings and generate identifiers dynamically. This makes it possible to use any string as common string.

Finally, the PR adds more common strings that weren't added previously due to the fact that common strings didn't support non-ident strings.

@kjarosh kjarosh requested a review from moulins April 17, 2026 22:50
@kjarosh kjarosh added A-core Area: Core player, where no other category fits T-refactor Type: Refactor / Cleanup labels Apr 17, 2026
Comment thread core/macros/src/lib.rs
@moulins
Copy link
Copy Markdown
Contributor

moulins commented Apr 18, 2026

I'm not a fan of how this moves the actual struct CommonStrings definition outside of the avm_string/common.rs file...
Instead, what if common_string_to_ident was directly exposed as a proc-macro? Then define_common_strings can stay as a normal decl-macro, and include things like common_string_to_ident!($field): AvmString<'gc> in its expansion.

@kjarosh
Copy link
Copy Markdown
Member Author

kjarosh commented Apr 18, 2026

I'm not a fan of how this moves the actual struct CommonStrings definition outside of the avm_string/common.rs file

Doesn't it make more sense for the structure to be defined in the same file it's used? I think it makes more sense for istr! to be in the same place as define_common_strings!.

@kjarosh kjarosh added the waiting-on-review Waiting on review from a Ruffle team member label Apr 18, 2026
@moulins
Copy link
Copy Markdown
Contributor

moulins commented Apr 18, 2026

Well, ideally istr! would also be a decl-macro defined in terms of common_string_to_ident!, but this can't be done easily as the short form (istr!("foo")) isn't hygienic.

Doesn't it make more sense for the structure to be defined in the same file it's used?

With your changes, the declaration also get split: e.g. define_common_strings! still depends on having ASCII_CHARS_LEN and ASCII_CHARS in scope, which makes understanding how the code works much more difficult imho.

Sure, this could be fixed by moving the entirety of the common.rs file inside the proc-macro, but that would makes thing even more opaque to the average reader (and would probably worsen IDE experience).

@kjarosh
Copy link
Copy Markdown
Member Author

kjarosh commented Apr 18, 2026

Sure, this could be fixed by moving the entirety of the common.rs file inside the proc-macro, but that would makes thing even more opaque to the average reader

I'm not convinced that's the case, the structure and its usage will be in the same file anyway. I think we can just more the existing code as-is before the struct. Doesn't make sense to make the implementation detail public. Placing definitions of istr and define_common_strings in completely different crates would be more misleading.

(and would probably worsen IDE experience).

In what way? I didn't notice anything particularly bad.

kjarosh added 3 commits April 19, 2026 00:17
This replaces the `define_common_strings!` macro-rules with a procedural
macro that automatically generates field names from the provided byte
string literals. This reduces duplication in `common.rs` by removing
the need to manually specify `str_` prefixes for every common string.
After refactoring `define_common_strings!` to a proc macro, we can
escape non-ident characters in strings and generate identifiers
dynamically. This makes it possible to use any string as common string.
Add more common strings that weren't added previously due to the fact
that common strings didn't support non-ident strings.
@moulins
Copy link
Copy Markdown
Contributor

moulins commented Apr 18, 2026

A "go-to" definition on the CommonStrings type won't bring anything useful: it'll point to the define_common_strings! { ... } macro call, and the actual type definition (including its constructor) will be completely hidden inside the proc-macro code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-core Area: Core player, where no other category fits T-refactor Type: Refactor / Cleanup waiting-on-review Waiting on review from a Ruffle team member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants