Big update to prettier-extension-monkeyc

flocsy said:

ERROR: fenix3: X/bin/optimized/group001-debug/source/source/MyApp.mc:139: Undefined symbol "App" detected

I'll see if I can repro this, but can you try adding --Eno-invalid-symbol to monkeyC.compilerOptions (Ctrl-, then type monkeyC.compilerOptions to find the setting), which should suppress the compile time error. Assuming it does, can you check it works at runtime? If it does (and I think it should), that means the error is a monkeyc compiler bug.

Either way, I should be able to work around it...

Why would I do that? I do the exact opposite: I set the compiler to be strict. If I would change that then I wouldn't know if the new code I add is OK or not. But this is not a compiler warning IMHO. The issue is this:

If you look at the optimized A.mc, it looks like:

// ...

App.AppBase.setProperty(key, val);

// ...

import Toybox.Application;
using Toybox.Application as App;
(:no_api2, :memory16K, :inline)
function setConfig(key as PropertyKeyType, val as PropertyValueType) as Void {
  App.AppBase.setProperty(key, val);
}

using Toybox.Application as App; // needed fot the inlined setConfig to work

flocsy said:

Why would I do that?

Because I was asking you to help me debug it :-)

flocsy said:

I do the exact opposite

Yes, of course. I just wanted more information about what was going on, because my previous testing had shown that you only need an import/using in *one* file to make it available everywhere - both at compile and runtime (In fact *you* had reported a bug where I wasn't identifying WatchUi as a toybox module, and it turned out to be because you didn't have an import of WatchUi - but you got no compile time errors, and the code worked at runtime).

But I've been able to repro your problem, so I no longer need you to do that.

But what's going on seems to be that sometimes, in some projects, the compiler auto-imports certain modules for you. And that made me think (incorrectly) that import/using was "global" - ie once you did it in one file, it applied to all of them. But for modules that don't get auto-imported (and I still don't know what triggers that), you really do have to import or use them in every file where you reference them.

So I'll change my lookup rules (again) to try to match that (but without the magic auto-import), and fix the inliner to add imports where necessary.

Btw, I don't understand why you're doing both "import Toybox.Application;" and "using Toybox.Application as App;". If I change App to Application in setConfig, and drop the using, it still works for me.

markw65 said:

Btw, I don't understand why you're doing both "import Toybox.Application;" and "using Toybox.Application as App;". If I change App to Application in setConfig, and drop the using, it still works for me

Also, I mentioned this in.a previous post, but note that YourApplication also extends AppBase, so you can just say YourApplication.setProperty(key, val), rather than App.AppBase.setProperty(key, val), and save another 4 bytes (assuming YourApplication doesn't need any qualifiers at the point where you're calling this).

flocsy said:

INFO: fenix6: source/Config.mc:55,17: While inlining getConfig

v2.0.18 is out, and should fix this one, the nested inline problem, and the comments on attributes issue.

I've not tried to fix the import issue, partly because I want to figure out what the best solution is size-wise, and partly because the workaround is trivial.

Just an idea: maybe when you import A.B.C, then also A.B gets imported?

Re: using Toybox.Application.App: you're right, it was there because when I wrote that part of the code I only had that, and not the import Toybox.Application, and it was there mainly because I uses Application in multiple places (all the function parameters: setConfig(key as App.PropertyKeyType, val as App.PropertyValueType). But since I already added import Application I could remove the "App.", and now because you turned my eyes also to MyApp.setProperty I could completely remove App. (I did see when you wrote that I can use MyApp.setProperty, I just didn't realize then that it also saves bytes. Thanks for pointing me to that again!) And still why did I use using? Well the obvious reason: good qa thinks about edge cases that the developer forgot to test ;)

Both inline nesting and comments after the annotation work now.

Not sure about the last sentence about the import issue: "want to figure out what the best solution is size-wise". What size are you talking about? Your plugins size? Or adding unnecessary imports/using to ONE file that doesn't really need it would increase the code size even if the same import was already used in another file? (BTW if this is true then I might need to reorganize my files/excludeAnnotations, because for example in case of the old devices that can only use AppBase.getProperty vs new devices that use Properties.getValue obviously only one import is needed for a specific device, and still the current code (because it uses excludeAnnotations) has both of them. It will complicate things a bit if I split these to 2 directories: source-no_api2 and source-has_api2 (especially that if I fix something in one place then I need to remember to do it in the other place as well) but if that saves bytes then I might do it) 

flocsy said:

Not sure about the last sentence about the import issue

I *was* concerned that redundant imports would have.a size impact (so I was looking at both adding the new import, and removing the possibly no-longer-needed import from the original file), but experiments so far suggest they don't. So I'm trying to figure out the best way to take advantage of that in general (ie inserting "import Toybox.Application.Properties" and using Properties instead of Application.Properties whenever possible).

flocsy said:

BTW if this is true then I might need to reorganize

So right now, I don't think you do. But hopefully, you wouldn't have to anyway, because the optimizer will already generate separate code for those cases, and once I complete the work above, it should produce optimal (wrt imports) code for each case.

[edit: I also forgot to mention that adding imports can break things. eg

module foo {
  enum { Properties = 1; }
  function bar() as Number { return Properties; }
}

If I add "import Toybox.Application.Properties;" at the top, the reference to Properties in bar resolves to the module Properties, not the enum.

Yes, really, it does.

In fact, it's worse than that. If I add "import Toybox.Application;" it breaks too (but "using Toybox.Application" does not).

So properly nested, perfectly working code can be broken by adding an extra import/using. And import is especially insidious, because it appears that all contained modules are also going to be pulled in.

So before I can add an extra using in a file, I have to check *all* of the file's symbols for potential conflicts. 

Fortunately, I can use "using Whatever as NameThatsNotAlreadyUsed;" to avoid issues like that - but I still have to build a table of names that aren't used, and then rewrite the inlined expression to *use* NameThatsNotAlreadyUsed... ]

I think Garmin should employ you as chief architect

OK, I have now one use case for this and it works, but I think now I'm pretty sure it's naming/meaning should be the other way around:

    (:memory16K)
    hidden function antId2Name() as String? {
        var name;
        name = antId2NameByProperty("s");
        if (name != null) {
            return name;
        }
        name = getConfigStr("n", DEFAULT_ALIAS);
        return "".equals(name) ? null : name;
    }
    (:memory32K)
    hidden function antId2Name() as String? {
        var name = antId2NameByProperty("s");
        if (name != null) {
            return name;
        }
        name = antId2NameByProperty("u");
        if (name != null) {
            return name;
        }
        name = getConfigStr("n", DEFAULT_ALIAS);
        return "".equals(name) ? null : name;
    }

    (:inline_memory32K)
    hidden function antId2NameByProperty(key as String) as String? { // 11130,4536 | 600,5027,2791,3347 => i: 5008,2759
        var result = null;
        var sensorsOrUsers = getConfigPropertyDictionaryArray(key, [] as PropertyDictionaryArray); // null : if sensorsOrUsers != null => []: -5
        for (var i = 0; i < sensorsOrUsers.size(); i++) {
            var sensorOrUser = sensorsOrUsers[i];
            var name = sensorOrUser["n"] as String;
            if (mSensor.deviceCfg.deviceNumber == sensorOrUser["i"] && !"".equals(name)) {
                // name can be null, but that's OK, we would anyway return null at the end of the function
                // we could return name here and save 8 bytes in memory32K, but inlining it in memory16 saves much more there
                result = name;
                break;
            }
        }
        return result;
    }

The above code works as intended: when in low memory machines it's inlined, because there it's only used once, when in higher memory machines it's not inlined because it's used twice. However the name "inline_memory32K" makes no sense to me. It really should do what it does now, when I annotate it with :memory_16K IMHO.

If you have a counter-example then share it please. If not, then I'd be happy if you can change it. Much more logical for future usage IMHO.

flocsy said:

If you have a counter-example then share it please. If not, then I'd be happy if you can change it. Much more logical for future usage IMHO.

Ok, I'll flip it in the next release. Your example makes sense.