Skip to content

Conversation

@cirras
Copy link
Collaborator

@cirras cirras commented Mar 15, 2024

This PR finishes implementing support for Delphi 12.

Copy link
Collaborator

@fourls fourls left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing - looks great for the most part! Just a couple of questions and a changelog improvement.

There's been a few changes in master since this branch forked, so would be good to get it rebased on master as well.

@fourls
Copy link
Collaborator

fourls commented Mar 26, 2024

Code change looks good, but it seems to have significantly increased the execution time of the delphi-checks tests - on GitHub Actions it's doubled, going from 4m 50s in master to 9m 46s on this branch. Ideally we'd get this back down to a similar amount of time before merging.

Initial analysis and profiling suggests that it's due to an uptick in the volume of runtime classes generated for weak alias types.

@cirras
Copy link
Collaborator Author

cirras commented Mar 27, 2024

Code change looks good, but it seems to have significantly increased the execution time of the delphi-checks tests - on GitHub Actions it's doubled, going from 4m 50s in master to 9m 46s on this branch. Ideally we'd get this back down to a similar amount of time before merging.

Initial analysis and profiling suggests that it's due to an uptick in the volume of runtime classes generated for weak alias types.

This was because we started generating weak aliases as part of TypeFactoryImpl construction.
We used to cache type alias classes per TypeAliasGenerator - that cache is now static and the classes are generated only once.

Copy link
Collaborator

@fourls fourls left a comment

Choose a reason for hiding this comment

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

All looks good now! Tests are back down to usual durations (perhaps even slightly faster).
Looks like we can close the book on Delphi 12 compatibility now 😄

@fourls fourls merged commit 58c92c3 into master Mar 27, 2024
@fourls fourls deleted the delphi12 branch March 27, 2024 01:37
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.

Model the length of open arrays as NativeInt under Delphi 12 Support TEXTBLOCK directive Handle NativeInt as a weak alias in Delphi 12

3 participants