[JASS] Leak or no Leak

[DigitalDemon]

function KillAll takes nothing returns nothing
    local group KG = GetUnitsSelectedAll(GetTriggerPlayer())
    local unit u
    local location Pos
    local effect x
    loop
    exitwhen CountUnitsInGroup(KG) <= 0
    set u = GroupPickRandomUnit( KG )
    set Pos = GetUnitLoc( u )
    call AddSpecialEffectLocBJ( Pos, "Abilities\\Spells\\Orc\\WarStomp\\WarStompCaster.mdl" )
    set x = GetLastCreatedEffectBJ()
    call KillUnit( u )
    call PolledWait(1.00)
    call RemoveUnit( u )
    call PolledWait(0.50)
    call DestroyEffect( x )
    call RemoveLocation( Pos )
    endloop
    call DestroyGroup( KG )
endfunction

Does the above leak? I think i've removed everything that could leak, but i'm not sure...
I only started learning Jass about 20 minutes ago... It's my first code which actually works.

 

[busterkomo]

Your handles aren't nulled( use

set var = null

Consider simplifying your code by setting:

set x = AddSpecialEffectLocBJ( Pos, "Abilities\\Spells\\Orc\\WarStomp\\WarStompCaster.mdl" )

Also, remove the BJ's, with what I wrote above, they become unnecessary(they may have have been unnecesarry before if AddSpecialEffectLoc also returned bj_lastCreatedEffect).
With removing BJ's(any any non-native for that matter, I can't think of a single one which is useful) your code becomes faster and more efficient(and sometimes longer).

 

[HINDYhat]

Hehe. Welcome to the Jassing community! Here are a couple of things you need to remember to make efficient code:

IMPORTANT:

• Remember to remove/destroy all handles after you're done using them (handle are all variable types except boolean, integer, real, string and code.
• Remember to null all local handles at the end of a function (parameters don't count).
• Try to avoid "BJ" calls in your functions (BJ's are functions from the blizzard.j Jass library). You should always try and inline these functions.

This being said, here's your code re-made, more efficient and explaining what I did:

function KillAll takes nothing returns nothing
    local group KG = GetUnitsSelectedAll(GetTriggerPlayer()) // This is a BJ function.
    local unit u
    local location Pos // Try to avoid using locations. You should use x,y real coordinates instead.
    local effect x
    loop
        exitwhen CountUnitsInGroup(KG) <= 0  // There is a much more efficient way to do this...
        set u = GroupPickRandomUnit( KG ) // Instead you should use FirstOfGroup()
        set Pos = GetUnitLoc( u )
        call AddSpecialEffectLocBJ( Pos, "Abilities\\Spells\\Orc\\WarStomp\\WarStompCaster.mdl" ) // Another BJ.
        set x = GetLastCreatedEffectBJ() // This is useless, you could've inlined it directly with the previous line.
        call KillUnit( u )
        call PolledWait(1.00)
        call RemoveUnit( u )
        call PolledWait(0.50)
        call DestroyEffect( x )
        call RemoveLocation( Pos )
    endloop
    call DestroyGroup( KG )
    // You're still leaking non-nulled handles here.
endfunction

And the rewritten code:

function KillAll_Child takes nothing returns nothing
    local unit u=bj_ghoul[100] // Here we store the temporary global into a local unit, so we can use waits and stuff.
    call DestroyEffect(AddSpecialEffect("Abilities\\Spells\\Orc\\WarStomp\\WarStompCaster.mdl",GetUnitX(u),GetUnitY(u))) // Inlining all previous useless calls with effect into one line.
    call KillUnit(u)
    call PolledWait(1.0)
    call RemoveUnit(u)
    set u=null // Nulling local handles.
endfunction
function KillAll takes nothing returns nothing
    local group KG = CreateGroup() // Inlining the GetSelectedUnits BJ.
    call SyncSelections() // See previous line.
    call GroupEnumUnitsSelected(KG,GetTriggerPlayer(),null) // See previous line.
    loop
        // We loop through the group in an interesting way... We set a
        // bj_ global (in this case, bj_ghoul[100]) to the first unit of the
        // group, and then we check if it's value is null. Finally we run through
        // the desired calls, and we remove the unit from the group. When
        // there are no more units in the group, FirstOfGroup(KG) will return null.
        set bj_ghoul[100]=FirstOfGroup(KG) // We have to use a temporary global variable here to carry the unit to another function.
        exitwhen bj_ghoul[100]==null
        call ExecuteFunc("KillAll_Child") // This call will start a new thread, so waits won't stack.
        call GroupRemoveUnit(KG,bj_ghoul[100]) // Removing the unit from the group is part of the loop
    endloop
    call DestroyGroup( KG ) // Removing handles.
    set KG=null // Nulling local handles.
endfunction

I haven't tested that, but it should work fine and be alot more efficient than your previous code. If you have any questions about it, feel free to ask.

 

[DigitalDemon]

Well i tried removing the BJs but i get an error saying "Cannot convert string to location" and "Cannot convert location to string"

function KillAll takes nothing returns nothing
    local group KG = GetUnitsSelectedAll(GetTriggerPlayer())
    local unit u
    local location Pos
    local effect x
    loop
    exitwhen CountUnitsInGroup(KG) <= 0
    set u = GroupPickRandomUnit( KG )
    set Pos = GetUnitLoc( u )
    set x = AddSpecialEffectLoc( Pos, "Abilities\\Spells\\Orc\\WarStomp\\WarStompCaster.mdl" )
    call KillUnit( u )
    call PolledWait(1.00)
    call RemoveUnit( u )
    call PolledWait(0.50)
    call DestroyEffect( x )
    call RemoveLocation( Pos )
    endloop
    call DestroyGroup( KG )
endfunction

Edit:
Oh, never mind. Without the BJ, the Location and Model get swapped around...!

 

[busterkomo]

In your KillAll_Child function he needed a wait before destroying the SE's(@Hindy).
Also, use coordinates like Hindy said(I will go into greater depth if you would like).

local real y
local real x2
set x2 = GetUnitX(u)
set y = GetUnitY(u)
//I would have normally used x as the coordinate, buy you used it as a SE
set x = AddSpecialEffect("Abilities\\Spells\\Orc\\WarStomp\\WarStompCaster.mdl", x2, y)

 

[HINDYhat]

Well usually, special effects just play their animation anyway even when directly removed. I'm not sure about this case, but meh.

 

[DigitalDemon]

Do coordinates respond faster or something? I thought 1 line of code is better than 2. Wouldn't you need to get the X and Y in seperate calls ?

EDIT:
Meh, i have to go out to see someone now but...
I'LL BE BACK!!!!11one1!!!

 

[busterkomo]

Sorry about the inconvenience but I edited my post after you two both posted after me so it looks odd. Locations are meant to be faster, but since they have to be removed(coordinates don't, they only exist in theory), coordinates are faster.
I'll repost some code down here for the convenience

set x = GetUnitX(u)
set y = GetUnitY(u)
set x = AddSpecialEffect("Abilities\\Spells\\Orc\\WarStomp\\WarStompCaster.mdl", x2, y)

 

[HINDYhat]

Coordinates (reals) are not handles. Locations are handles. And that being said, locations need to be cleaned up and nulled and stuff, PLUS natives almost always work with coordinate, not locations. You're much better off with coordinates.

The X and Y don't need to be stored in locals, unless you using them twice. In this case, nah you don't need to store them in locals.

 

[busterkomo]

Yes, HINDY is right so use

AddSpecialEffect("Abilities\\Spells\\Orc\\WarStomp\\WarStompCaster.mdl", GetUnitX(u), GetUnitY(u))

 

[DigitalDemon]

Thank you. +Rep now i have to go!

 

[PandaMine]

Well usually, special effects just play their animation anyway even when directly removed. I'm not sure about this case, but meh.

Depends on the effect. 90% of effects work like that, but some rare ones if you destroy straight away you dont see anything in which case you need to use a timer -> H2I (or any other method) -> destroy after wait