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

Draft Change: Use Squirrel version 3.0.7 #7708

Closed
wants to merge 2 commits into from
Closed

Conversation

nikolas
Copy link
Member

@nikolas nikolas commented Aug 29, 2019

Here's a branch to play around with that updates OpenTTD's modified
version of Squirrel 2.2.5 to a similarly modified version of
Squirrel 3.0.7.

This is a work-in-progress as the compiler warnings need to be addressed.
But, it's in a usable state to test out AIs and game scripts using
Squirrel 3.

@nikolas nikolas force-pushed the squirrel3 branch 4 times, most recently from 4001de9 to fa44532 Compare Aug 29, 2019
@nielsmh
Copy link
Contributor

@nielsmh nielsmh commented Aug 29, 2019

Fails on Windows since it compiles with SQUNICODE defined, which defined SQChar as wchar_t and becomes incompatible.

I think this is a high risk change not worth pursuing, it will most likely break a large number of existing AI and GS. In my opinion, if even a single existing addon breaks this can't be merged.

@LordAro
Copy link
Member

@LordAro LordAro commented Aug 29, 2019

In my opinion, if even a single existing addon breaks this can't be merged.

Agreed, unless the old behaviour can be restored

I'd like to be annoying and ask that adding squirrel 3 and making modifications to it should be separate commits. Breaks the "every commit should compile" rule, but I think it would be worth it to make it more obvious which are our modifications

@nikolas
Copy link
Member Author

@nikolas nikolas commented Aug 29, 2019

I think this is a high risk change not worth pursuing, it will most likely break a large number of existing AI and GS. In my opinion, if even a single existing addon breaks this can't be merged.

Absolutely this will break AIs that depend on Squirrel 2 features. For example, AdmiralAI breaks because it uses for(local c = 0; c < vargc; c++) {, and vargc is no longer a keyword in Squirrel 3.

I don't intend this to be merged any time soon. Rather, this is an experimental branch for anyone who wants to test things in Squirrel 3.

@nikolas nikolas force-pushed the squirrel3 branch 5 times, most recently from 44214e3 to 3fd4b91 Compare Aug 30, 2019
@nikolas
Copy link
Member Author

@nikolas nikolas commented Aug 30, 2019

I've separated this out into two commits on @LordAro's suggestion. It's easy to see the modifications now. That was a big challenge with updating this in the first place.

@nikolas nikolas force-pushed the squirrel3 branch 2 times, most recently from f343372 to 6335dfc Compare Aug 30, 2019
@nikolas nikolas changed the title Change: Use Squirrel version 3.0.7 Draft Change: Use Squirrel version 3.0.7 Sep 17, 2019
nikolas added 2 commits Nov 21, 2019
@LordAro
Copy link
Member

@LordAro LordAro commented Sep 24, 2020

Mm. A fun experiment I think, but I don't see this going any further - not unless you want to reimplement all the "removed" features from Squirrel 2 -> 3 :)

It's possible there are other improvements that could be taken from SQ3, but those need to be rather more selective than this!

@LordAro LordAro closed this Sep 24, 2020
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.

None yet

4 participants