Big update to prettier-extension-monkeyc

markw65 said:

I don't see where the 1 byte comes from with 4.1.7.

Mystery solved.

In 4.1.5 every function allocates space for locals, even if there aren't any (there's an "incsp 0" for functions with no variables). In 4.1.7 it drops the incsp if there are no variables, saving 2 bytes.

So in my test case, I had no variables, and two longs. So putting the long into a variable cost 2 bytes. If I'd started with a function containing other variables, it would have saved 3 bytes in 4.1.7 too.

But I've gone back through all the old compiler versions, and none behaves the way I thought they did... I must have just messed up my tests. So even more reason for creating automated tests to measure this.

So my pre-costs for Long and Double are incorrect (but incorrect in the right direction - it won't create variables that make code size worse; but it will *fail* to create variables that could make code size better).

If you shaving a 8 byte long you lose precision.  More so with a double.  Strings are a bit more interesting, as you'd see different memory between the sim and a real device.  In one case, an empty string would take one byte, and in the other, where memory was allocated by the 32 bit word. it would take 4.  And grow by 4 each time the string wouldn't fit in 4,8,12,16,etc  bytes..

I really wouldn't trust a difference of a few bytes, as how memory is allocated comes into play.

Since getConfigPropertyDictionaryArray is explicitly typed as being able to return null, isn't

 for (var i = 0; i < users.size(); i++) {

an even bigger issue?

But you should be able to silence the error via:

if ((user as PropertyDictionary?) != null) { //

Btw, I'll remind you that this error is one *you* suggested :-)

But I admit I don't much like this solution. I see lots of code doing things like:

if (x != null) {
  ...
  if (x != null) {
    ...
  }
}

and I really would like to eliminate the (inner) test in cases like that. And even in your case... if user *can* be null, why not declare it as such?

The reason for the warning is that we know the sdk is incorrectly typed in many cases, and there's nothing a user can do about that. But for your own code, why not type it correctly?

jim_m_58 said:

really wouldn't trust a difference of a few bytes, as how memory is allocated comes into play

Again, you're missing the point.

The size of the string is completely irrelevant. The only thing that matters is how big the code is that accesses it.

The size of the string is what it is - there's nothing I can do about that. But if I make the code that accesses it smaller, thats a good thing, regardless of the size of the string.

flocsy said:

Indeed I have, but 7 colors (5 + transparent for zone 0 and dark red for 6).

Why do you need transparent? And on a side note, this encoding scheme doesn't actually work for transparent. It's going to get encoded as 0b111111, which will be decoded as 0x00ffffff; ie white.

I originally used transparent too, but when I wanted to drop the count to 5, I realized there was no need for it; just don't draw that zone at all. After reorganizing the code a bit, there was actually less code. Obviously, I don't know exactly what your code is doing... but surely there's a way to avoid drawing something thats going to end up being transparent anyway?

That still leaves you with 6 colors, but maybe there's a 5 bit encoding that works? After all, 5 bits to encode 6 colors leaves a lot of redundancy...

The six colors in binary are:

    function rgb5(color as Graphics.ColorType) as Number {
        return (
            ((color & 0xc00000) >> 19) +
            ((color & 0x4000) >> 12) +
            ((color & 0xc0) >> 6)
        );
    }
    (:inline)
    private function getColor(zone as Number) as Number {
        var field = mColors >> (zone * 5);
        return (
            (field & 0x03) * 0x55 +
            ((field & 0x04) + (~zone & 4) * 2) * 0x1540 +
            (field & 0x18) * 0xaa000
        );
    }
    private const mColors as Number =
        (rgb5(Graphics.COLOR_LT_GRAY) << 0) +
        (rgb5(Graphics.COLOR_BLUE) << 5) +
        (rgb5(Graphics.COLOR_GREEN) << 10) +
        (rgb5(Graphics.COLOR_YELLOW) << 15) +
        (rgb5(Graphics.COLOR_RED) << 20) +
        (rgb5(Graphics.COLOR_DK_RED) << 25);

I think in my example it went 1cstep too far :) My code does explicitly declare the function as such that can return Null. BUT the optimizer is clever enough to see what I saw only after having a second look at this, that the only way it can return Null is by returning the 2nd argument, but that is never null in my code (currently)

So i think the right thing to do is to eliminate the if from the optimized code, because there it is really not needed.

And I can see why i would get a warning or an info about it, because from the current state of my code (that is: i only pass non null values as 2nd parameter) it is not necessary to check if the returned value is null.

But i would not log it as an error, it is not an error, no matter how i look at it (even in your example, where I agree that the inner check is completely unnecessary and i would expect to see a warning about it, but not an error)

In my case however I am hesitant to decide if warning or info is better, because the code (the example that calls the function and treats the return value + the code of the called function) is good. I think because I send a const as the 2nd argument probably a warning makes sense. 

Probably this question is too philosophical though: what should the basic assumption be when someone (or prettier) does a code review? I think most programmers would spot that null can be returned and (at least in my mind) that would grant a null check. But i agree that prettier went one step further and it is correct in deducing that with the actual circumstances in my current code it'll never return Null. And this is good! It means prettier does it's job better than i would have :) 

I think it's impossible to decide the philosophical question, but the bottom line is that even though I set in prettier's settings the level to error, this is not a case where an error should be reported. A warning is enough.

The funny thing is that i also realized yesterday that I never use the transparent :) because when the background color of the datafield is black it didn't work for some reason so in my code i have something like:

return zone>0&zone<7 ? ZONES[zone] : backgroundColor:

So in fact I removed the transparent from the array, refactored the condition in the above code, but i also have one more place where ZONES is used, there i had to add -1 to the index, but since I already had var one=1 it was still beneficial.

I'll try your new code tonight :) I like your way of thinking! Actually I think I just understood now why I am here (wasting my time on CIQ) : back in the old days i really liked assembly and hardware close C development and hacking my code now gives me satisfaction (especially when enough bytes are saved to enable me to squeeze in one more feature for the "old" devices)

Actually I'll try something else: the right-most blue bit... First I thought it's easy: if zone is 1 then it's 1, otherwise it's 0. So that can be even easier recovered from the index :) Then I thought that I can even try how it looks without that but. It'll be a little bit (punch intended) less blue but probably close enough ( side note, before I saw that you used yellow i used orange, but i changed it now to yellow so probably nobody will notice if i remove some amount of blue)

flocsy said:

I think in my example it went 1cstep too far :) My code does explicitly declare the function as such that can return Null

I think one of us is confused... so let me say what I think, and you can tell me why I'm wrong.

I agree that you declared the function as possibly returning null. But (as far as I can see) that has nothing to do with the null test in your code.

If getConfigPropertyDictionaryArray returns null, then the variable users is null. But that's not what you're testing. You're testing users[i]. And users[i] is declared as PropertyDictionary, and PropertyDictionary cannot be null.

Not only that, but as I said above, if users is null then users.size() is going to crash, before you get to the line that tests users[i] (and of course, if you *did* get there, then users[i] would crash too).

So my point above was that if you don't want the warning you should declare it as returning Array<PropertyDictionary?> (or maybe Array<PropertyDictionary?>?).

Also note  this has nothing to do with the optimizer figuring out that getConfigPropertyDictionaryArray never actually returns null (and it definitely doesn't do that).

In fact, I'm pretty sure that Garmin's type checker should produce an error when you say users.size() without guarding for null - so are you not using it here?

This is something my type checker doesn't do yet - it does *know* that it could be null, but it doesn't issue a diagnostic. There are a few other places where I don't yet issue diagnostics too. My goal is to be able to turn off Garmin's type checker; but we're not there yet.

flocsy said:

First I thought it's easy: if zone is 1 then it's 1, otherwise it's 0. So that can be even easier recovered from the index

I did consider that too; but (~zone & 4) is cheaper than (zone==1?1:0); and I can't see a better way of converting zone==1 to one or zero

Note that (zone==1 as Number) *looks* like it works, and keeps the type checker happy, but if you try adding that to a number, it'll crash (Number + Boolean isn't supported), and if you use bitwise-or instead, you'll get a boolean, rather than a numeric result because bitwise or of a Number and a Boolean produces a boolean.

But yes, dropping a bit is always an option if you're not too worried about the exact colors...