search

dnSpyEx / dnSpy

Unofficial revival of the well known .NET debugger and assembly editor, dnSpy
GNU General Public License v3.0
7.02k stars 462 forks source link

Decompiler should use for(;;) instead of while() #313

Closed pardeike closed 7 months ago

pardeike commented 7 months ago

dnSpyEx version

6.5.0

Describe the Bug

ILSpy:

public override Vector3 OffsetAtTick(int tick, PawnDrawParms parms)
{
    // ...
    for (int i = 0; i < part.keyframes.Count; i++)
    {
        if (tick <= part.keyframes[i].tick)
        {
            keyframe2 = part.keyframes[i];
            if (i > 0)
            {
                keyframe = part.keyframes[i - 1];
            }
            break;
        }
    }
    // ...
}

dnSpy:

public override Vector3 OffsetAtTick(int tick, PawnDrawParms parms)
{
    // ...
    int i = 0;
    while (i < this.part.keyframes.Count)
    {
        if (tick <= this.part.keyframes[i].tick)
        {
            keyframe2 = this.part.keyframes[i];
            if (i > 0)
            {
                keyframe = this.part.keyframes[i - 1];
                break;
            }
            break;
        }
        else
        {
            i++;
        }
    }
    // ...
}

The IL (from ILSpy:

.method /* 06001652 */ public hidebysig virtual 
    instance valuetype [UnityEngine.CoreModule]UnityEngine.Vector3 OffsetAtTick (
        int32 tick,
        valuetype Verse.PawnDrawParms parms
    ) cil managed 
{
    // Method begins at RVA 0x8877c
    // Header size: 12
    // Code size: 339 (0x153)
    .maxstack 4
    .locals /* 110004D5 */ init (
        [0] class Verse.Keyframe,
        [1] class Verse.Keyframe,
        [2] float32,
        [3] int32
    )

    // if (tick <= part.keyframes[0].tick)
    /* 0x00088788 03                 */ IL_0000: ldarg.1
    /* 0x00088789 02                 */ IL_0001: ldarg.0
    /* 0x0008878A 7BBC120004         */ IL_0002: ldfld class Verse.AnimationPart Verse.AnimationWorker::part /* 040012BC */
    /* 0x0008878F 7BB2120004         */ IL_0007: ldfld class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe> Verse.AnimationPart::keyframes /* 040012B2 */
    /* 0x00088794 16                 */ IL_000c: ldc.i4.0
    /* 0x00088795 6F200F000A         */ IL_000d: callvirt instance !0 class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe>::get_Item(int32) /* 0A000F20 */
    /* 0x0008879A 7BAE120004         */ IL_0012: ldfld int32 Verse.Keyframe::tick /* 040012AE */
    /* 0x0008879F 3017               */ IL_0017: bgt.s IL_0030

    // return part.keyframes[0].offset;
    /* 0x000887A1 02                 */ IL_0019: ldarg.0
    /* 0x000887A2 7BBC120004         */ IL_001a: ldfld class Verse.AnimationPart Verse.AnimationWorker::part /* 040012BC */
    /* 0x000887A7 7BB2120004         */ IL_001f: ldfld class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe> Verse.AnimationPart::keyframes /* 040012B2 */
    /* 0x000887AC 16                 */ IL_0024: ldc.i4.0
    /* 0x000887AD 6F200F000A         */ IL_0025: callvirt instance !0 class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe>::get_Item(int32) /* 0A000F20 */
    /* 0x000887B2 7BAF120004         */ IL_002a: ldfld valuetype [UnityEngine.CoreModule]UnityEngine.Vector3 Verse.Keyframe::offset /* 040012AF */
    /* 0x000887B7 2A                 */ IL_002f: ret

    // if (tick >= part.keyframes[part.keyframes.Count - 1].tick)
    /* 0x000887B8 03                 */ IL_0030: ldarg.1
    /* 0x000887B9 02                 */ IL_0031: ldarg.0
    /* 0x000887BA 7BBC120004         */ IL_0032: ldfld class Verse.AnimationPart Verse.AnimationWorker::part /* 040012BC */
    /* 0x000887BF 7BB2120004         */ IL_0037: ldfld class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe> Verse.AnimationPart::keyframes /* 040012B2 */
    /* 0x000887C4 02                 */ IL_003c: ldarg.0
    /* 0x000887C5 7BBC120004         */ IL_003d: ldfld class Verse.AnimationPart Verse.AnimationWorker::part /* 040012BC */
    /* 0x000887CA 7BB2120004         */ IL_0042: ldfld class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe> Verse.AnimationPart::keyframes /* 040012B2 */
    /* 0x000887CF 6F210F000A         */ IL_0047: callvirt instance int32 class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe>::get_Count() /* 0A000F21 */
    /* 0x000887D4 17                 */ IL_004c: ldc.i4.1
    /* 0x000887D5 59                 */ IL_004d: sub
    /* 0x000887D6 6F200F000A         */ IL_004e: callvirt instance !0 class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe>::get_Item(int32) /* 0A000F20 */
    /* 0x000887DB 7BAE120004         */ IL_0053: ldfld int32 Verse.Keyframe::tick /* 040012AE */
    /* 0x000887E0 3228               */ IL_0058: blt.s IL_0082

    // return part.keyframes[part.keyframes.Count - 1].offset;
    /* 0x000887E2 02                 */ IL_005a: ldarg.0
    /* 0x000887E3 7BBC120004         */ IL_005b: ldfld class Verse.AnimationPart Verse.AnimationWorker::part /* 040012BC */
    /* 0x000887E8 7BB2120004         */ IL_0060: ldfld class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe> Verse.AnimationPart::keyframes /* 040012B2 */
    /* 0x000887ED 02                 */ IL_0065: ldarg.0
    /* 0x000887EE 7BBC120004         */ IL_0066: ldfld class Verse.AnimationPart Verse.AnimationWorker::part /* 040012BC */
    /* 0x000887F3 7BB2120004         */ IL_006b: ldfld class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe> Verse.AnimationPart::keyframes /* 040012B2 */
    /* 0x000887F8 6F210F000A         */ IL_0070: callvirt instance int32 class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe>::get_Count() /* 0A000F21 */
    /* 0x000887FD 17                 */ IL_0075: ldc.i4.1
    /* 0x000887FE 59                 */ IL_0076: sub
    /* 0x000887FF 6F200F000A         */ IL_0077: callvirt instance !0 class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe>::get_Item(int32) /* 0A000F20 */
    /* 0x00088804 7BAF120004         */ IL_007c: ldfld valuetype [UnityEngine.CoreModule]UnityEngine.Vector3 Verse.Keyframe::offset /* 040012AF */
    /* 0x00088809 2A                 */ IL_0081: ret

    // Keyframe keyframe = part.keyframes[0];
    /* 0x0008880A 02                 */ IL_0082: ldarg.0
    /* 0x0008880B 7BBC120004         */ IL_0083: ldfld class Verse.AnimationPart Verse.AnimationWorker::part /* 040012BC */
    /* 0x00088810 7BB2120004         */ IL_0088: ldfld class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe> Verse.AnimationPart::keyframes /* 040012B2 */
    /* 0x00088815 16                 */ IL_008d: ldc.i4.0
    /* 0x00088816 6F200F000A         */ IL_008e: callvirt instance !0 class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe>::get_Item(int32) /* 0A000F20 */
    /* 0x0008881B 0A                 */ IL_0093: stloc.0
    // Keyframe keyframe2 = part.keyframes[part.keyframes.Count - 1];
    /* 0x0008881C 02                 */ IL_0094: ldarg.0
    /* 0x0008881D 7BBC120004         */ IL_0095: ldfld class Verse.AnimationPart Verse.AnimationWorker::part /* 040012BC */
    /* 0x00088822 7BB2120004         */ IL_009a: ldfld class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe> Verse.AnimationPart::keyframes /* 040012B2 */
    /* 0x00088827 02                 */ IL_009f: ldarg.0
    /* 0x00088828 7BBC120004         */ IL_00a0: ldfld class Verse.AnimationPart Verse.AnimationWorker::part /* 040012BC */
    /* 0x0008882D 7BB2120004         */ IL_00a5: ldfld class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe> Verse.AnimationPart::keyframes /* 040012B2 */
    /* 0x00088832 6F210F000A         */ IL_00aa: callvirt instance int32 class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe>::get_Count() /* 0A000F21 */
    /* 0x00088837 17                 */ IL_00af: ldc.i4.1
    /* 0x00088838 59                 */ IL_00b0: sub
    /* 0x00088839 6F200F000A         */ IL_00b1: callvirt instance !0 class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe>::get_Item(int32) /* 0A000F20 */
    /* 0x0008883E 0B                 */ IL_00b6: stloc.1
    // for (int i = 0; i < part.keyframes.Count; i++)
    /* 0x0008883F 16                 */ IL_00b7: ldc.i4.0
    /* 0x00088840 0D                 */ IL_00b8: stloc.3
    // if (tick <= part.keyframes[i].tick)
    /* 0x00088841 2B49               */ IL_00b9: br.s IL_0104
    // loop start (head: IL_0104)
        /* 0x00088843 03                 */ IL_00bb: ldarg.1
        /* 0x00088844 02                 */ IL_00bc: ldarg.0
        /* 0x00088845 7BBC120004         */ IL_00bd: ldfld class Verse.AnimationPart Verse.AnimationWorker::part /* 040012BC */
        /* 0x0008884A 7BB2120004         */ IL_00c2: ldfld class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe> Verse.AnimationPart::keyframes /* 040012B2 */
        /* 0x0008884F 09                 */ IL_00c7: ldloc.3
        /* 0x00088850 6F200F000A         */ IL_00c8: callvirt instance !0 class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe>::get_Item(int32) /* 0A000F20 */
        /* 0x00088855 7BAE120004         */ IL_00cd: ldfld int32 Verse.Keyframe::tick /* 040012AE */
        /* 0x0008885A 302C               */ IL_00d2: bgt.s IL_0100

        // keyframe2 = part.keyframes[i];
        /* 0x0008885C 02                 */ IL_00d4: ldarg.0
        /* 0x0008885D 7BBC120004         */ IL_00d5: ldfld class Verse.AnimationPart Verse.AnimationWorker::part /* 040012BC */
        /* 0x00088862 7BB2120004         */ IL_00da: ldfld class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe> Verse.AnimationPart::keyframes /* 040012B2 */
        /* 0x00088867 09                 */ IL_00df: ldloc.3
        /* 0x00088868 6F200F000A         */ IL_00e0: callvirt instance !0 class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe>::get_Item(int32) /* 0A000F20 */
        /* 0x0008886D 0B                 */ IL_00e5: stloc.1
        // if (i > 0)
        /* 0x0008886E 09                 */ IL_00e6: ldloc.3
        /* 0x0008886F 16                 */ IL_00e7: ldc.i4.0
        /* 0x00088870 312D               */ IL_00e8: ble.s IL_0117

        // keyframe = part.keyframes[i - 1];
        /* 0x00088872 02                 */ IL_00ea: ldarg.0
        /* 0x00088873 7BBC120004         */ IL_00eb: ldfld class Verse.AnimationPart Verse.AnimationWorker::part /* 040012BC */
        /* 0x00088878 7BB2120004         */ IL_00f0: ldfld class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe> Verse.AnimationPart::keyframes /* 040012B2 */
        /* 0x0008887D 09                 */ IL_00f5: ldloc.3
        /* 0x0008887E 17                 */ IL_00f6: ldc.i4.1
        /* 0x0008887F 59                 */ IL_00f7: sub
        /* 0x00088880 6F200F000A         */ IL_00f8: callvirt instance !0 class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe>::get_Item(int32) /* 0A000F20 */
        /* 0x00088885 0A                 */ IL_00fd: stloc.0
        // break;
        /* 0x00088886 2B17               */ IL_00fe: br.s IL_0117

        // for (int i = 0; i < part.keyframes.Count; i++)
        /* 0x00088888 09                 */ IL_0100: ldloc.3
        /* 0x00088889 17                 */ IL_0101: ldc.i4.1
        /* 0x0008888A 58                 */ IL_0102: add
        /* 0x0008888B 0D                 */ IL_0103: stloc.3

        // for (int i = 0; i < part.keyframes.Count; i++)
        /* 0x0008888C 09                 */ IL_0104: ldloc.3
        /* 0x0008888D 02                 */ IL_0105: ldarg.0
        /* 0x0008888E 7BBC120004         */ IL_0106: ldfld class Verse.AnimationPart Verse.AnimationWorker::part /* 040012BC */
        /* 0x00088893 7BB2120004         */ IL_010b: ldfld class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe> Verse.AnimationPart::keyframes /* 040012B2 */
        /* 0x00088898 6F210F000A         */ IL_0110: callvirt instance int32 class [mscorlib]System.Collections.Generic.List`1<class Verse.Keyframe>::get_Count() /* 0A000F21 */
        /* 0x0008889D 32A4               */ IL_0115: blt.s IL_00bb
    // end loop

    // float t = (float)(tick - keyframe.tick) / (float)(keyframe2.tick - keyframe.tick);
    /* 0x0008889F 03                 */ IL_0117: ldarg.1
    /* 0x000888A0 06                 */ IL_0118: ldloc.0
    /* 0x000888A1 7BAE120004         */ IL_0119: ldfld int32 Verse.Keyframe::tick /* 040012AE */
    /* 0x000888A6 59                 */ IL_011e: sub
    /* 0x000888A7 6B                 */ IL_011f: conv.r4
    /* 0x000888A8 07                 */ IL_0120: ldloc.1
    /* 0x000888A9 7BAE120004         */ IL_0121: ldfld int32 Verse.Keyframe::tick /* 040012AE */
    /* 0x000888AE 06                 */ IL_0126: ldloc.0
    /* 0x000888AF 7BAE120004         */ IL_0127: ldfld int32 Verse.Keyframe::tick /* 040012AE */
    /* 0x000888B4 59                 */ IL_012c: sub
    /* 0x000888B5 6B                 */ IL_012d: conv.r4
    /* 0x000888B6 5B                 */ IL_012e: div
    /* 0x000888B7 0C                 */ IL_012f: stloc.2
    // return def.scale * Vector3.Lerp(keyframe.offset, keyframe2.offset, t);
    /* 0x000888B8 02                 */ IL_0130: ldarg.0
    /* 0x000888B9 7BBA120004         */ IL_0131: ldfld class Verse.AnimationDef Verse.AnimationWorker::def /* 040012BA */
    /* 0x000888BE 7BB8120004         */ IL_0136: ldfld float32 Verse.AnimationDef::scale /* 040012B8 */
    /* 0x000888C3 06                 */ IL_013b: ldloc.0
    /* 0x000888C4 7BAF120004         */ IL_013c: ldfld valuetype [UnityEngine.CoreModule]UnityEngine.Vector3 Verse.Keyframe::offset /* 040012AF */
    /* 0x000888C9 07                 */ IL_0141: ldloc.1
    /* 0x000888CA 7BAF120004         */ IL_0142: ldfld valuetype [UnityEngine.CoreModule]UnityEngine.Vector3 Verse.Keyframe::offset /* 040012AF */
    /* 0x000888CF 08                 */ IL_0147: ldloc.2
    /* 0x000888D0 28140C000A         */ IL_0148: call valuetype [UnityEngine.CoreModule]UnityEngine.Vector3 [UnityEngine.CoreModule]UnityEngine.Vector3::Lerp(valuetype [UnityEngine.CoreModule]UnityEngine.Vector3, valuetype [UnityEngine.CoreModule]UnityEngine.Vector3, float32) /* 0A000C14 */
    /* 0x000888D5 28CB05000A         */ IL_014d: call valuetype [UnityEngine.CoreModule]UnityEngine.Vector3 [UnityEngine.CoreModule]UnityEngine.Vector3::op_Multiply(float32, valuetype [UnityEngine.CoreModule]UnityEngine.Vector3) /* 0A0005CB */
    /* 0x000888DA 2A                 */ IL_0152: ret
} // end of method AnimationWorker_Keyframes::OffsetAtTick

How To Reproduce

Decompile a method with that specific code.

Expected Behavior

It would nice if dnSpy could use for loops instead of while loops

Actual Behavior

Its creating sub-optimal source

Additional Context

I get constant shade at the RimWorld discord when I promote dnSpy mainly because of issues like this. I love and will continue to use dnSpy when coding with Harmony, so this is more of a heads-up.

funkvps commented 7 months ago

Have you tried the new-ilspy branch? From description, it seems new version ilspy will produce for loop, than new-ilspy branch should be ok

pardeike commented 7 months ago

Yes, that fixes the code generation.

riQQ commented 7 months ago

See #5 for the current status of integrating the latest ILSpy version.