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

Wrong Code Gen for Abstract Static Functions in Module #860

Closed
frederikaalund opened this issue Jun 8, 2019 · 6 comments
Closed

Wrong Code Gen for Abstract Static Functions in Module #860

frederikaalund opened this issue Jun 8, 2019 · 6 comments
Labels

Comments

@frederikaalund
Copy link
Contributor

frederikaalund commented Jun 8, 2019

I think that I've found an issue with the code generation. The following code reproduces the issue:

package TestPackage

module TestModule
	protected static thistype array instances
	abstract static protected function _construct(int i) returns thistype
	constant int i
	
	static protected function initStaticInstances()
		instances[0] = _construct(42)

class TestClass
	use TestModule

	construct(int i)
		this.i = i

	// THIS FUNCTION HAS WRONG CODE GEN
	override static protected function _construct(int i) returns thistype
		return new TestClass(i)

init
	TestClass.initStaticInstances()

This produces the following function (from compiled.j):

function TestClass_TestModule__construct takes integer i returns integer
	return 0
endfunction

The body is just return 0. It should have been the jass equivalent of return new TestClass(i).


The motivation for a static _construct is to be able to construct instances of thistype in a Module. Currently, the code new thistype(42) doesn't compile.

@Frotty
Copy link
Member

Frotty commented Jun 8, 2019

I think abstract static is not intended to work, this is a missing compilation error.
What stops you from using a non static function and override it?

@Frotty Frotty added the bug label Jun 8, 2019
@frederikaalund
Copy link
Contributor Author

frederikaalund commented Jun 8, 2019

Thanks for the prompt reply.

What stops you from using a non static function and override it?

I want to construct a Class instance from within a Module. That is, what I really want is to be able to write new thistype(42) within a Module. Unfortunately, this doesn't currently work. Instead, I tried to use abstract static protected function _construct(int i) returns thistype as a work-around but got bitten by bad code gen.

I think abstract static is not intended to work, this is a missing compilation error.

I had a feeling this was the case. Admittedly, abstract static is somewhat special-purpose. It seems to be a debated issue in other languages as well.

@Frotty
Copy link
Member

Frotty commented Jun 8, 2019

I want to construct a Class instance from within a Module. That is, what I really want is to be able to write return new thistype(42) within a Module. Unfortunately, this doesn't currently work. Instead, I tried to use abstract static protected function _construct(int i) returns thistype as a work-around but got bitten by bad code gen.

Yes, I don't know if it makes sense to support creating new objects of thistype in module.
Anyway you can solve it with a non-static method as I mentioned like this e.g.:

interface MyCommonInterface

module A
	abstract function instance() returns MyCommonInterface

class B implements MyCommonInterface
	use A

	override function instance() returns MyCommonInterface
		return new B()

But then you could just as well sue abstract class :D

@frederikaalund
Copy link
Contributor Author

frederikaalund commented Jun 8, 2019

Thanks again for the quick reply and for the code sample. :)

The problem with instance() is that it requires an instance to create an instance (like the hen and egg problem). How would I use it from within the Module?


Let me try to provide a motivating example:


/** Automatically create a class instance for each player
  *
  * Your class must:
  *   • Declare the constructor(s) private
  *   • Set this.p in the constructor(s)
  *   • Call initForAll() before using the class
  */
public module AlwaysOnePerPlayer
	protected static thistype array instances
	constant player p
		
	/** Call this once. E.g., in an init block */
	static protected function initForAll()
		for i = 0 to bj_MAX_PLAYER_SLOTS - 1
			instances[i] = new thistype(players[i])

	/** Always returns non-null */
	static function getFor(player p) returns thistype
		return instances[p.getId()]


public class Character
	use AlwaysOnePerPlayer
	private unit u = null
	private int kills = 0
	private int deaths = 0

	private construct(player p)
		this.p = p

init
    Character.initForAll()

Now I can call Character.getFor(p) from other places and now that I will always get an instance. Many maps typically have a central character (e.g., a hero) that can be conveniently referenced this way.

@Frotty
Copy link
Member

Frotty commented Jun 8, 2019

Hm yeah, I suppose it would save on some boilerplate code if you do this to many classes.
I reckon it depends on design choices and the classical approach is simply, to have 1 such class which you init like your module, and then you save the other player-dependent class, in that "player data class", which also provides nice encapsulation.
The choice of having a character instance with a null handle u vs null Character reference until that unit is decided, could also be discussed in this example.

@frederikaalund
Copy link
Contributor Author

Thanks for considering the use case. :)

Hm yeah, I suppose it would save on some boilerplate code if you do this to many classes.
I reckon it depends on design choices and the classical approach is simply, to have 1 such class which you init like your module, and then you save the other player-dependent class, in that "player data class", which also provides nice encapsulation.

Yes, the key idea is to shave off boilerplate code through abstraction. I already have three classes in my map that use a variant of the above-mentioned module. There is the CameraControl class for instance. It centralizes per-player camera management. E.g., for cinematics, presentations, or "fixed zoom".

The choice of having a character instance with a null handle u vs null Character reference until that unit is decided, could also be discussed in this example.

That is nicely spotted! :) The short code example was too contrived, I guess (I tried to remove irrelevant parts). The actual Character class doesn't simply use unit u = null; it preserves unit state (e.g., XP) between character reassignments (hero repick).

@peq peq closed this as completed in 68fb4bc Jun 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants