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

Modified compiler to allow dynamic arrays within structs #164

Merged

Conversation

monkey0506
Copy link
Contributor

This allows dynamic arrays to be included in structs. Preliminary testing shows this works. Tested saving and loading as well.

@gurok
Copy link
Contributor

gurok commented Jul 8, 2014

Umm... can you keep the spacing the same? I know, I know, I get annoyed with the spaces/tabs mixture too. It just makes the diff much tidier that way.

@monkey0506
Copy link
Contributor Author

Sorry, that's something in my Visual Studio settings, I think.
On Jul 8, 2014 12:33 AM, "gurok" notifications@github.com wrote:

Umm... can you keep the spacing the same? I know, I know, I get annoyed
with the spaces/tabs mixture too. It just makes the diff much tidier that
way.


Reply to this email directly or view it on GitHub
#164 (comment)
.

@monkey0506
Copy link
Contributor Author

I'll look at it.
On Jul 8, 2014 12:33 AM, "gurok" notifications@github.com wrote:

Umm... can you keep the spacing the same? I know, I know, I get annoyed
with the spaces/tabs mixture too. It just makes the diff much tidier that
way.


Reply to this email directly or view it on GitHub
#164 (comment)
.

@ghost
Copy link

ghost commented Jul 8, 2014

Meanwhile, how to see changes ignoring whitespace difference: add "/?w=1" to the link:

https://github.com/monkey0506/ags/commit/5347be46294abc0777c9094150ddbf7a902613d0/?w=1

@gurok
Copy link
Contributor

gurok commented Jul 8, 2014

Wow, CW, that's awesome!

@ghost
Copy link

ghost commented Jul 8, 2014

@ghost
Copy link

ghost commented Jul 8, 2014

Re arrays, they seem to work. I wonder why Chris Jones did not add them earlier. Is it simply forgetfulness or there's a hidden problem? Need to investigate this a bit more.

@rofl0r

This comment was marked as abuse.

@ghost
Copy link

ghost commented Jul 8, 2014

how are dynamic arrays stored ? if it's only a pointer to the storage (by ref) it might work out, but if the elements are stored in the struct itself (by val) there's no way to know in advance how much space the struct will take.

They are supposed to be stored by reference, as well as all other "managed" types in AGS script.
They worked as global/local variables, but for some reason compiler did not allow to put them in user-defined structs, which was pretty illogical, because other references to game types could be.

@monkey0506
Copy link
Contributor Author

AFAICT, yes, the dynamic arrays are only stored by reference, while the actual memory is allocated in the managed object pool (to provide reference counting and garbage collection). With the introduction of by-reference user types by @gurok , this seemed a logical next step.

Since the arrays are stored/passed by reference, this should only ever increase the size of a struct by 4 bytes, as with any other pointer. There was no indication that I could find as to why CJ disallowed them from being included in structs, but I agree that this should be very thoroughly tested before pulling it in. This seemed the most appropriate route to do that.

@monkey0506
Copy link
Contributor Author

I rebased the commit to ignore whitespace changes.

@ghost
Copy link

ghost commented Jul 12, 2014

Call me nagger, but the file has spaces for indentation, and your commit introduces tabs.
Also, may you please remove "(no whitespace changes)" part from commit's comment? It makes little sense on its own.

E: I tried different combinations, and I don't see any problem with this feature so far.

@monkey0506
Copy link
Contributor Author

Changing the Visual Studio settings to use spaces introduced over 2000 changes, so I'm a bit skeptical that there weren't already tabs used throughout the file. I went ahead and checked out the cached copy of the file and manually redid my work on top of it. This diff should only reflect the actual, relevant changes, and I made sure to use spaces not tabs.

@ghost
Copy link

ghost commented Jul 13, 2014

Hmm, ok. We might do an automatic replacement at some point maybe.

Thank you, and sorry for bothering.

ivan-mogilko added a commit that referenced this pull request Jul 13, 2014
Modified compiler to allow dynamic arrays within structs
@ivan-mogilko ivan-mogilko merged commit d65cce0 into adventuregamestudio:develop-3.3.1 Jul 13, 2014
@monkey0506 monkey0506 deleted the struct_dyn_arrays branch July 14, 2014 01:53
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