search

adventuregamestudio / ags

AGS editor and engine source code
Other
707 stars 159 forks source link

AGS 4: remake DynamicArray.Length pseudo-property getter as a object member function #2559

Closed ivan-mogilko closed 2 weeks ago

ivan-mogilko commented 3 weeks ago

@fernewelten , I would very much like to know your opinion on this.

I've been revisiting Array.Length pseudo property recently, and it hit me that it's implemented as a static function. Somehow I did not pay attention to this back when it was done (by #1256). I noticed that my proposal in this comment mentions "function name accepting 1 argument", so maybe this is what caused it, or maybe there were other considerations too.

But thinking about this now, this does not seem convenient, as such interpretation breaks parsing rules: you have object.property syntax, but it's been treated as if it were function(arg). The new compiler has to have a separate case for this, where it has to generate a function call in a separate "outside of system" way.

I am not completely sure, but I suspect that this may cause trouble would we have more pseudo-properties like that, or would a compiler's parser be structured differently, making it more complicated to inject special handling like that.

My proposal is to modify this by treating the property's getter as a object's member function, just like properties (attributes) are normally treated in AGS.

What I did was this: Instead of generating a call to a pseudo-static-function, the parser generates an actual readonly attribute "T[]::Length", where "T[]" is a dynamic array type. The getter is substituted by a generated function called "__Builtin_DynamicArray::get_Length". The same symbol is used for each such generated attribute. This is where it goes slightly away from your system, as the function does not have a reference to the struct it belongs to, as it cannot belong to multiple structs. But this does not cause any trouble at the moment. I suppose that for the perfect simulation we could have a hidden built-in parent class for arrays, and attach this attribute there. But I did not go that far.

Note that no special function call is generated after this, instead I just let parser to continue its work and deal with the attribute usage in a normal way. This seems to be a more natural way of handling this.

In regards to backwards compatibility. This feature did not introduce any new opcodes, thankfully, it was only a function name. All it relies on is an actual function linked by the engine. I kept old static function registration within the engine for the time being, in case it is used to run old compiled script.

As an extra change, this PR contains support for Array.Length added to the old compiler. I was doing this as an experiment, when I stumbled on the issue above.

Added new extension name to compilers, called "DYNARRAY_LENGTH", this may be used to test if compiler supports .Length pseudo property.

ericoporto commented 3 weeks ago

Just to comment that the failures in the GHA CI are just that it can't run the test game because they are pre-compiled with the older __Builtin_DynamicArrayLength^1 method, but this is alright, it's just rebuilding the test games after this PR.

ivan-mogilko commented 2 weeks ago

@fernewelten idk if you read my original PR's description, I had some change in mind and redid this in more ways:

Instead of generating a call to a pseudo-function, the parser generates an actual readonly attribute "T[]::Length", where "T[]" is a dynamic array type. The getter is substituted by a generated function called "__Builtin_DynamicArray::get_Length". The same symbol is used for each such generated attribute. This is where it goes slightly away from your system, as the function does not have a reference to the struct it belongs to, as it cannot belong to multiple structs. But this does not cause any trouble at the moment. I suppose that for the perfect simulation we could have a hidden built-in parent class for arrays, and attach this attribute there. But I did not go that far.

Note that no special function call is generated after this, instead I just let parser to continue its work and deal with the attribute usage in a normal way. This seems to be a more natural way of handling this.

I also kept an original function reg in the engine, that has a very small cost in size of code, and would let run previously compiled scripts with array.Length in them.

ivan-mogilko commented 2 weeks ago

Okay, I will merge this in. In my opinion, generating "automatic" types and properties in parser is a better approach than secretly generating a call to a completely different type of function. Hypothetically, we might even have a proper generic parent for arrays this way later, maybe this is a way for (limited) generics in AGS script.