Big update to prettier-extension-monkeyc

I found another bug in v2.0.18, when the inlined function isn't removed:

class Foo {
    var bar as Bar;
    
    (:memory32K, :color)
    public function compute(info as Activity.Info) as Void {
        var extHr = computeExtHR();
        var heartRateZone;
        heartRateZone = computeHrZone(extHr);
        bar.computeHrZone(heartRateZone);
    }

    (:memory32K, :inline)
    hidden function computeHrZone(heartRate as Number) as Number {
        var z;
        if (heartRateZones != null) {
            for (z = 0; z <= 5 && heartRate > heartRateZones[z]; z++) {
            }
        } else {
            z = -1;
        }
        return z;
    }
}

class Bar {
    public function computeHrZone(z as Number) {}
}

It's only used in 1 place, and the inlining is done, but in this case the original function isn't removed for some reason. All the above code is within the same class.

Update: I think I found the "reason". I have the same method name computeHrZone in this class and in another class. When I renamed the method in the other class to something else then it works. I hope this helps you to find the bug. Garmin doesn't let me to edit the above code box :(

I raise again the possible feature to be able to tell to inline a function only if it's being used less than N times. I had 2 cases where I had a function inlined, and with the time the code involved and I called it more times at which point it was no more efficient to inline it. In both cases the function didn't change, so I could've known in advance (in both cases it was pretty obvious because they were big functions and the only reason I did not manually inline them at the beginning was that I had 2 different versions depending on memory size, so I could annotate them with :inline_max_1). Currently when I added another call I didn't get any warning and it was still inlined in all places, so the sice increased significantly (which helped to notice the problem). So IMHO the feature (:inline_max_<N>) or even the more specific, however arguably the most relevant (:inline_once) could help in these cases. The max_<N> is even helpful in case of "small" functions like:

flocsy said:

I hope this helps you to find the bug

This is a feature, not a bug, I'm afraid. And unless I do end up writing my own type checker, it's likely to remain one.

The problem is that if you have "class A (function foo(){} } and class B {function foo(){} }" and somewhere you do "var x;... x.foo();" then I don't know which foo was called, because x could be either type (*you* may know its type, but the optimizer doesn't).

If each foo is only ever called implicitly (ie from other methods of A or B either as "foo()" or as "self.foo()", then I should be able to figure it out. Any other kind of call, and both foo methods get marked as called (and therefore not removable).

I think I'm not *too* far from being able to implement a "trust types" option, which would let me figure out which foo was called as long as x was a class or module variable which had a type declaration on it. But that still won't help with locals (which, as established earlier, can't be explicitly typed).

I see, but I think that even if you don't know the type (which is not the case here, becuase bar is a member), there are some cases that are easy to differentiate:

foo() and self.foo() should be the only cases considered as self.foo() was called.

variable.foo() should not be considered as such no matter if variable is a member variable or a local variable, because in both cases it's 99% that it's calling another class' public function, which most probably can't be inlined anyway (can it?)

In theory I could probably do:
var trick = self;
trick.foo();

but let's be realistic, this is an edge case, and even then trick.foo() probably can be considered as calling another class' object's public foo function, so not inlining it is OK I guess.

Anyway, I renamed my other foo(), so I'm fine now ;)

Found another thing, probably a bug:

class Foo {
    var mResult as String;
    
    function compute() {
        // ...
        mResult = mTicker <= FOUND_SECONDS ? extHr as String : strfHrZone(confFormat, extHr, extZone, computeIntHR(info));
    }

    (:api1_3, :inline)
    hidden function strfHrZone(format as String, extHr as AlphaNumeric, extZone as AlphaNumeric, intHr as AlphaNumeric) as String {
        var name = mName != null ? mName : "";
        return Lang.format(format as String, [extHr, extZone, name, intHr]);
    }
}

INFO: fenix6: source/Foo.mc:100,64: While inlining strfHrZone: This function can only be inlined in statement, assignment, or return contexts

However when I change the line to the following, then it works:

flocsy said:

So I see no other reason but that there's some bug and the trinary operator confuses it

Well, the INFO tells you it has to be in statement, assignment, or return context to be inlined. When you put it in assignment context, it gets inlined. This is exactly how it's supposed to work.

Assignment context means it's the right hand side of an assignment. If it's a subexpression of the right hand side, its in expression context. Similarly for return context.

Here's the reason. If I have x = foo(); and foo() looks liked

function foo() {
  // do lots of crazy stuff
  return <some expression>
}

I can inline it by simply inserting:

{
  // do lots of crazy stuff
  x = <some expression>
}

in place of the original assignment. If its part of a subexpression that doesn't work. In some cases I could do something similar, but eg if it was x = y.z + foo(), and I try to write it as:

{
  // do lots of crazy stuff
  x = y.z + (<some expression>)
}

There's a danger that "lots of crazy stuff" changes y.z. And if I tried it with x = a ? foo() : 0, we'd be executing all that crazy stuff even when a was false.

So, for now, I just restrict it to cases that are easy to deal with. That way *you* can rewrite the tricky cases since you know where side effects matter, and where they don't.

Ok, that makes sense. Any chance you could improve the info message to be more specific? BTW I'm trying to think how to refactor this so that a) it can be inlined, b) it doesn't increase the code size. Any idea?

OK I made a "compromise" :) 

This does increase the non-optimized code size by 8 bytes, but the optimized code size is decreased by 13 bytes and the data also by 22, which is a lot of gain!

        if (mTicker <= FOUND_SECONDS) {
            mResult = extHr as String;
         } else {
            mResult = strfHrZone(confFormat, extHr, extZone, computeIntHR(info));
         }

        if (mTicker > FOUND_SECONDS) {
            extHr = strfHrZone(confFormat, extHr, extZone, computeIntHR(info));
        }
        mResult = extHr as String;

BOOOM: forums.garmin.com/.../optimal-monkey-c

flocsy said:

BOOOM

Wow. Well, first impression... my project doesn't build with the type new checker enabled. When I disable it, my fr235 build goes from 13.3k with my optimizer, to 13.6k with theirs at -O2. So I'm still doing things theirs doesn't...

edit: one of the bugs