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

Variadic Source Generator #166

Closed
wants to merge 17 commits into from

Conversation

LilithSilver
Copy link
Contributor

@LilithSilver LilithSilver commented Oct 28, 2023

Currently, the Arch source-generation is pretty clunky. There's no compilation validation at all, making edits on the codebase hell.

This PR introduces a "variadic source generator", similar to variadic templates in C++. It makes variadic generics.. well, generic!

For example, you can specify a variadic like so...

public static partial class EntityExtensions
{
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    [Pure]
    [Variadic(nameof(T1), 2, 25)]
    public static bool Has<T0, T1>(this Entity entity)
    {
        var world = World.Worlds[entity.WorldId];
        return world.Has<T0, T1>(entity);
    }
}

This signature will be the very first variadic generated, so it'll generate <T0, T1, T3> onwards up to <T0 ... T24> (total count 25).

Additional features can be added through comments. For example, this directive:

// [Variadic: CopyArgs(component)]
world.Add(entity, component__T0, component__T1);

Will produce:

// [Variadic: CopyArgs(component)]
world.Add(entity, component__T0, component__T1, ..., component__T24);

Using this directive:

var component__T0 = Get<T0>();
// [Variadic: CopyLines(component)]
var component__T1 = Get<T1>();

Will produce:

var component__T0 = Get<T0>();
// [Variadic: CopyLines(component)]
var component__T1 = Get<T1>();
var component__T2 = Get<T2>();
var component__T3 = Get<T3>();
...

It additionally puts each variadic in separate files like this:
image
This enables easier debugging instead of one monolithic file.

@LilithSilver LilithSilver changed the title Variadic sourcegen Variadic Source Generator Oct 28, 2023
@genaray
Copy link
Owner

genaray commented Oct 30, 2023

Sounds great! One question tho. Is the same output produced? E.g. many source generated overloads make use of the batched operations e.g. : Index<T0,,,T25>(out indexOne, indextwo, ..). ^^

@LilithSilver
Copy link
Contributor Author

Sounds great! One question tho. Is the same output produced? E.g. many source generated overloads make use of the batched operations e.g. : Index<T0,,,T25>(out indexOne, indextwo, ..). ^^

Yep, I'm using the original sourcegen as a guide! Variable names might be different but the code should semantically be the same.

Cool, I'll convert everything over then.

@LilithSilver LilithSilver marked this pull request as ready for review November 1, 2023 12:54
@TwistyGolf
Copy link

Is there a reason for the change of variable names from t0, t1 etc to Component__T0, Component__T1?
This will require all users to update the usages, unless I'm missing something

@LilithSilver
Copy link
Contributor Author

Is there a reason for the change of variable names from t0, t1 etc to Component__T0, Component__T1?

This is because it's much easier to have a common variable/parameter format for the variadic source generator to hook into. We could switch back to t0Component etc but it would more effort and be more finicky, since it doesn't map directly to the type.

This will require all users to update the usages, unless I'm missing something

There are only two situations which require user intervention here:

  • The user was specifying parameters by name instead of by order. I don't think this is very common for the generic overloads, but it will cause them to change the specified parameter names on upgrade.
  • The user was using Component<...> or EntityComponent<...> directly and setting t0, t1, etc. In the case, those old fields were breaking standard of upper case field names for public fields.

Copy link
Owner

@genaray genaray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally had some time to look at the pr ^^
Looks good so far, kinda confusing since its source-gen. So i requested a few changes for clarity. Also... do we really require those doubled .Has e.g. methods? We could make it so that we just mark the single generics with variadic?

src/Arch.Benchmarks/QueryBenchmark.cs Outdated Show resolved Hide resolved
src/Arch.SourceGen/CopyArgsAlgorithm.cs Outdated Show resolved Hide resolved
src/Arch.SourceGen/CopyArgsAlgorithm.cs Outdated Show resolved Hide resolved
src/Arch.SourceGen/VariadicGenerator.cs Outdated Show resolved Hide resolved
src/Arch.SourceGen/VariadicGenerator.cs Show resolved Hide resolved
src/Arch.SourceGen/VariadicGenerator.cs Outdated Show resolved Hide resolved
src/Arch.SourceGen/VariadicGenerator.cs Outdated Show resolved Hide resolved
@LilithSilver
Copy link
Contributor Author

OK, fixed what I could til I get more info from genaray, and did another documentation pass. Thanks for the feedback, both of you!

@genaray
Copy link
Owner

genaray commented Nov 14, 2023

Alright the previous concerns were fixed ^^
I still need to take another look at one or two other parts, that should be done in the following days.

@LilithSilver
Copy link
Contributor Author

Closing this for now since it's gone stale, and I don't have time to keep an eye on it -- if at some point you want to integrate this, feel free to re-open!

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