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

clean up Generator top-level and use ArgumentParser #617

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

mayoff
Copy link
Contributor

@mayoff mayoff commented Nov 25, 2024

This PR makes several changes to the Generator command.

The Generator command now uses the ArgumentParser module and an AsyncParsableCommand conformance to parse the command line and run async code. This cleans up command line parsing, improves the help message, and makes it much easier to add more command line options in the future. It also eliminates the hack of using a DispatchSemaphore to synchronously wait for a Task to finish.

This PR introduces a new GeneratorCommand type which conforms to AsyncParsableCommand. The @main annotation makes GeneratorCommand the entry point for the program. I renamed main.swift to Generator.swift because we can't have a main.swift if we use the @main annotation.

This PR also introduces a new Generator type, which holds every formerly-global variable that was declared in main.swift. All of these variables are now constant (let) instead of mutable (var).

I moved many formerly global functions into extensions of Generator so they can continue accessing these variables easily.

I changed many of the helper functions to private. I found some functions that were defined in one file but only used in a different file, and moved their definitions into the file where they are used (and made them private or fileprivate).

I removed a few completely unused functions and variables.

This PR eliminates the fatal compile-time errors that prevented Generator from compiling at all with strict concurrency checking enabled. There are still several concurrency warnings, but it builds and runs successfully.

I made the changes as a series of simple, somewhat trivial commits. The Generator program builds and produces unchanged output after each individual commit in this PR. I'm happy to squash the commits if you prefer, but I thought it would be easier to review as separate commits.

Rob Mayoff added 30 commits November 24, 2024 15:28
This simplifies checking whether a Godot type is a struct.
Rob Mayoff added 12 commits November 25, 2024 00:06
This commit renames main.swift to Generator.swift, which is necessary to use the @main annotation.

This commit attaches the @main annotation to GeneratorCommand and
removes the top-level code that used a DispatchSemaphore hack to wait
synchronously for an async to finish.
Error in Windows CI: "error: property wrappers are not yet supported in top-level code"
Windows CI error: error: cannot convert value of type 'WritableKeyPath<_, _>' to expected argument type '(JGodotBuiltinClass) -> JGodotBuiltinClass'
@mayoff mayoff force-pushed the generator-AsyncParsableCommand branch from e567058 to 7823a01 Compare November 25, 2024 07:03
@mayoff mayoff force-pushed the generator-AsyncParsableCommand branch from 14acea2 to 0634ee6 Compare November 25, 2024 07:27
@samdeane
Copy link
Contributor

samdeane commented Nov 29, 2024

Whilst you're at it, can I suggest moving the Generator source code into Sources/Generator? The Xcode project could move into Sources/Generator too, for now, and be fixed up so that it can find its source.

See also #620.

@migueldeicaza
Copy link
Owner

I am not ready to move the Generator to a new location, but happy to review this version.

@migueldeicaza
Copy link
Owner

I reviewed, applied and tested up to:

armpro$ git cherry-pick f691f71dbf4e98d5ae5c532f6985ebf1f80f4a9b
Auto-merging Generator/Generator/ClassGen.swift
[main a870c31] convert isStructMap Dictionary to isStruct function
 Author: Rob Mayoff <mayoff@dqd.com>
 Date: Sun Nov 24 15:19:47 2024 -0600
 5 files changed, 32 insertions(+), 33 deletions(-)
armpro$ git cherry-pick 6ef1232ea9cc65f40d30e5c052d17dde0a8e2658
[main 1b95d1b] move actual generation into async Generator.run
 Author: Rob Mayoff <mayoff@dqd.com>
 Date: Sun Nov 24 15:34:06 2024 -0600
 1 file changed, 25 insertions(+), 16 deletions(-)
armpro$ git cherry-pick 1415b9c5a7997b9f8ccfccf7008cf4ef2ab8569a
[main ddc204b] move generateNativeStructures into Generator
 Author: Rob Mayoff <mayoff@dqd.com>
 Date: Sun Nov 24 15:43:53 2024 -0600
 1 file changed, 120 insertions(+), 118 deletions(-)
armpro$ git cherry-pick e58c8769103c7a99f820217156861ae8cc3d44f9
Auto-merging Generator/Generator/ClassGen.swift
[main 5e8e94e] remove referenceTypes (just use classMap instead)
 Author: Rob Mayoff <mayoff@dqd.com>
 Date: Sun Nov 24 15:46:45 2024 -0600
 1 file changed, 1 insertion(+), 10 deletions(-)
armpro$ git cherry-pick 6212a6f045ccba7f5ed3ddc474a8b138a9500a78
Auto-merging Generator/Generator/ClassGen.swift
[main 9057911] move generateCtorPointers into Generator
 Author: Rob Mayoff <mayoff@dqd.com>
 Date: Sun Nov 24 15:47:35 2024 -0600
 2 files changed, 8 insertions(+), 6 deletions(-)
armpro$ git cherry-pick 3c3176554d745f1c409bbe1401e951d6e4be410b
Auto-merging Generator/Generator/ClassGen.swift
[main b5242af] remove unused tree global
 Author: Rob Mayoff <mayoff@dqd.com>
 Date: Sun Nov 24 15:51:05 2024 -0600
 1 file changed, 5 deletions(-)
armpro$ git cherry-pick 85507c0f01d06d493213ebb91a7b9b4de9277a3b
Auto-merging Generator/Generator/ClassGen.swift
[main 1747c83] remove unused typeToChildren global
 Author: Rob Mayoff <mayoff@dqd.com>
 Date: Sun Nov 24 15:54:13 2024 -0600
 1 file changed, 27 deletions(-)

Will continue later.

@migueldeicaza
Copy link
Owner

Generally, one of the problems with these cleanup patches is that I have a fork of SwiftGodot with various in-progress patches that I need for Godot on iPad, and these become very time consuming and cumbersome to merge.

I think that directionally this is a good set of changes, but I can not land right now, so I will have to do it in steps.

return lambdaFull
}

private func generateSignals (_ p: Printer,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this is out of date now with the generic signal implementation.

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.

3 participants