Add IsDefined builtin and AddToPool/#pool #40

StanHash · 2019-02-22T13:33:46Z

They work essentially the same way they did in legacy EA.

IsDefined(Name) expands to 1 if a definition or macro (~~why are those different things~~) of given name exists, expands to 0 otherwise.
AddToPool(tokens...) expands to the name of a label referring to data described by the listed tokens (that would be pooled by a subsequent #pool directive).
AddToPool(tokens..., Alignment) is the same as above, with the additional guarantee that the pooled data will be aligned with requested Alignment. This one is isn't in legacy EA.

Pool stuff is still a bit jank as pool labels are still affected by scope shenanigans (so in order for all of this to work, the #pool directive needs to be in a label scope that is visible to the AddToPool macros). We could make this slightly better if #37 gets implemented. Dumped pooled data is now parsed as if label scope is the same as the one from the AddToPool expression.

Pool labels are internally under the form __POOLED$<number>. They have a $ character in them to guarantee no user label can conflict with pool labels.

(I need to figure out how I can expand full expressions within builtin macros so I can implement more of the old stuff (Stuff like Signum, Switch, and maybe a variant of IsDefined that takes a second integer parameter that checks for a macro with that parameter count specifically)).

Crazycolorz5 · 2019-02-22T13:41:40Z

ColorzCore/Preprocessor/Directives/PoolDirective.cs

+
+        public Maybe<ILineNode> Execute(EAParser p, Token self, IList<IParamNode> parameters, MergeableGenerator<Token> tokens)
+        {
+            foreach (List<Token> line in p.PooledLines)


This will write the pooled lines in reverse order. Since order isn't meant to matter, it's probably fine, but I want to make sure it's intensional.

Interesting quirk here, since the tokens are tagged with their original location, "relocating" them via pool shouldn't affect error parsing.

I think. Please test error messages to confirm they error on the AddToPool() and not on the #pool.

Error messages are indeed being linked to the AddToPool invocation (and not the #pool). However it may be a good idea to add in a note: pooled at xyz or something but I don't really know how we would go about this?

That would probably be extraneous/difficult to implement. (hm, if I were to implement it, I'd probably do some kind of algebraic effects on the error handler to wrap adding a "pooled at" message to the default handler... within the pool directive scope. but endsidethought)

Crazycolorz5 · 2019-02-22T13:42:21Z

ColorzCore/Parser/EAParser.cs

@@ -79,6 +82,9 @@ public EAParser(Dictionary<string, IList<Raw>> raws, Log log, DirectiveHandler d
            Definitions = new Dictionary<string, Definition>();
            Inclusion = ImmutableStack<bool>.Nil;
            this.directiveHandler = directiveHandler;
+
+            PooledLines = new List<List<Token>>();


Error or warning upon completion of parsing if the pool is not emptied at the end.

Alternatively, thoughts on associating pooled lines to scope, and thus erroring if there are still pooled lines at the end of a scope?

Or do you think pooled lines should be able to escape to outer scopes?

I don't like this. Putting extra data somewhere without the user explicitly asking for it sounds like a bad idea.

I think you misunderstand me. I mean if I have

{ AddToPool(blah) } #pool

Should this error? Arguably, pooled data should be required only within its scope, so lack of a #pool in the scope may be an error, so we may want to enforce pooling on a scope level... was my thought

🤔 I think pooling outside of scope that way is fine, I think it is only problematic if the #pool were to be inside a child scope and not parent scope... But even then maybe it isn't idk

{ AddToPool(blah) } #pool // ok

AddToPool(blah) { #pool // maybe not ok? }

(but yes I did misread/read too quickly and thought doing silent stuff instead of err sorry).

Wait I think I see a (technical) issue now: you wouldn't be able to access scope local labels referenced within the AddToPool expression if the corresponding #pool was outside... ugh

Let's associate pooled lines with the scope they're used in, then, and error at the end of a scope if there are undumped pooled lines.

Crazycolorz5 · 2019-02-22T13:46:17Z

ColorzCore/Parser/Macros/AddToPool.cs

+        {
+            List<Token> line = new List<Token>(6 + parameters[0].Count);
+
+            string labelName = ParentParser.MakePoolLabelName();


To not overload the role of EAParser doing everything (which it is dangerously close to), let's try to have this as a static method here.

Crazycolorz5 · 2019-02-22T13:50:37Z

ColorzCore/Parser/Macros/IsDefined.cs

+        {
+            if (parameters[0].Count != 1)
+            {
+                // TODO: err somehow


Probably do some param checking in the invocation of built in macros akin to how it is for preproc defs.

(you don't need to address this comment if you don't want to; I intend to)

ColorzCore/Parser/Macros/IsDefined.cs

Crazycolorz5 · 2019-02-22T15:15:20Z

ColorzCore/Preprocessor/Directives/PoolDirective.cs

            foreach (List<Token> line in p.PooledLines)
            {
-                tokens.PrependEnumerator(line.GetEnumerator());
+                MergeableGenerator<Token> tempGenerator = new MergeableGenerator<Token>(line);


I think I miscommunicated. I meant you didn't neeeeed to emit a node. But if this works it's probably fine? BlockNode might have issues with scope, but if it doesn't, this is probably fine to leave as-is.

I made this mostly because I felt like it was cleaner, not because of any specific thing you mentioned (no offense).

Tho in the end this proved to be useful to fix scope stuff (as I ended up needing to parse those "manually" anyway to be able to pass scope from AddToPool to the pooled statements).

Use an explicit for loop to avoid the case where AddToPool is nested (eg
AddToPool(UNIT whatever AddToPool(REDA whatever); UNIT)
)
In the interpretation of this, I believe the latter add to pool would get added to the pooled lines, but never iterated over, and then cleared in line 32.

Crazycolorz5

I don't think it makes much sense to have a EAParser level Pool and have to associate each pooled line with a scope -- it makes keeping track of which line is in which scope hard (imo).

What I was thinking was each scope (as a class) has a list of pooled lines. Errors would occur upon scope destruction... which is currently at the end of ParseBlock only, I think (not looking too closely rn).

I believe that has organizational advantages over this approach, such as dumping a pool is simply dumping the lines in the current scope.

Feel free to convince me this (current approach) is the better approach, of course.

StanHash · 2019-02-22T16:25:09Z

I just kind of want to be able to do things like this:

#ifndef PooledRedaDefined
    #define PooledRedaDefined
    #define PooledReda(X, Y) "AddToPool(REDA [(X), (Y)] 0 0 0 0)"
#endif // PooledRedaDefined

// [Whatever Chapter Things]

scSomeReinf: {
    SPTR s2 unUnits
    CALL scLoadReinforcements

    NoFade
    ENDA

unUnits:
    UNIT [...] 1 PooledReda(x1, y1) [...]
    UNIT [...] 1 PooledReda(x2, y2) [...]
    UNIT

}

scSomeReinf2: {
    SPTR s2 unUnits
    CALL scLoadReinforcements

    NoFade
    ENDA

unUnits:
    UNIT [...] 1 PooledReda(x3, y3) [...]
    UNIT [...] 1 PooledReda(x4, y4) [...]
    UNIT

}

// [More chapter things]

#pool

Having a pool at the end of each scope here would be annoying to the user imo, and I don't think it makes things any clearer or safer (again, to the user).

EDIT: Also to be somewhat consistent with nl!EA behaviour, which allows this.

Crazycolorz5 · 2019-02-22T16:28:26Z

Makes sense. So we want to allow for bleeding out of scopes...
But there is the issue you mentioned before about pooling something that refers to a local label. And I can see error messages from that being more cryptic (as the user, you'd see the variable defined/bound RIGHT THERE)

I think personally it makes more sense to force more often pooling rather than have cases that hace issues like that.

Edit: I'm okay with diverging from original EA behavior on this one because a) it wasn't commonly used b) it had a lot of issues (which is what we are pointing out with it right now). B probably contributed to A tbh.

StanHash · 2019-02-22T16:29:12Z

I fixed that in my last commit (pooled statements have the scope of the AddToPool expression, not the #pool directive)

Crazycolorz5 · 2019-02-22T16:34:36Z

Ah, I misunderstood your fix. So each pooled line gets parsed as if in the closure of its original AddToPool, then the pooled label gets added into the global namespace?

Hm, yeah I guess that works.

StanHash · 2019-02-22T16:36:28Z

The pooled label gets into that same scope too (as the label definition is just part of the pooled statements).

Crazycolorz5 · 2019-02-22T16:40:15Z

ColorzCore/Preprocessor/Directives/PoolDirective.cs

            foreach (List<Token> line in p.PooledLines)
            {
-                tokens.PrependEnumerator(line.GetEnumerator());
+                MergeableGenerator<Token> tempGenerator = new MergeableGenerator<Token>(line);


Use an explicit for loop to avoid the case where AddToPool is nested (eg
AddToPool(UNIT whatever AddToPool(REDA whatever); UNIT)
)
In the interpretation of this, I believe the latter add to pool would get added to the pooled lines, but never iterated over, and then cleared in line 32.

Crazycolorz5 · 2019-02-22T16:45:07Z

going back to add things into the scope seems unintuitive to me (as there's no other language mechanism to add definitions to a scope once outside it), but again, pooled labels are referred to at only one location, so there shouldn't be any fishy behavior from it

StanHash added 3 commits February 22, 2019 13:09

(builtin-macros) IsDefined

708b837

(builtin-macros) non-singleton String for consistency

2462c96

(builtin-macros) AddToPool and pool

7a2f5b4

Crazycolorz5 requested changes Feb 22, 2019

View reviewed changes

StanHash added 3 commits February 22, 2019 15:36

(builtin-macros) move pool label generated to AddToPool

c7dc132

(builtin-macros) #pool expands to BlockNode instead of tokens

90ffd4b

(builtin-macros) proper errors on unpooled data

8786463

Crazycolorz5 reviewed Feb 22, 2019

View reviewed changes

(builtin-macros) fix scope issues with AddToPool/pool

2fd3e56

Crazycolorz5 requested changes Feb 22, 2019

View reviewed changes

(builtin-macros) fix nested AddToPool

22784a7

Crazycolorz5 approved these changes Mar 21, 2019

View reviewed changes

Crazycolorz5 merged commit 9e815f2 into FireEmblemUniverse:master Mar 21, 2019

StanHash deleted the builtin-macros branch January 31, 2021 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IsDefined builtin and AddToPool/#pool #40

Add IsDefined builtin and AddToPool/#pool #40

StanHash commented Feb 22, 2019 •

edited

Loading

Crazycolorz5 Feb 22, 2019

Crazycolorz5 Feb 22, 2019

StanHash Feb 22, 2019

Crazycolorz5 Feb 22, 2019

Crazycolorz5 Feb 22, 2019

Crazycolorz5 Feb 22, 2019

StanHash Feb 22, 2019

Crazycolorz5 Feb 22, 2019

StanHash Feb 22, 2019

StanHash Feb 22, 2019

Crazycolorz5 Feb 22, 2019

Crazycolorz5 Feb 22, 2019

Crazycolorz5 Feb 22, 2019

Crazycolorz5 Feb 22, 2019

StanHash Feb 22, 2019

Crazycolorz5 Feb 22, 2019

Crazycolorz5 left a comment

StanHash commented Feb 22, 2019 •

edited

Loading

Crazycolorz5 commented Feb 22, 2019 •

edited

Loading

StanHash commented Feb 22, 2019 •

edited

Loading

Crazycolorz5 commented Feb 22, 2019

StanHash commented Feb 22, 2019

Crazycolorz5 Feb 22, 2019

Crazycolorz5 commented Feb 22, 2019

Add IsDefined builtin and AddToPool/#pool #40

Add IsDefined builtin and AddToPool/#pool #40

Conversation

StanHash commented Feb 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crazycolorz5 left a comment

Choose a reason for hiding this comment

StanHash commented Feb 22, 2019 • edited Loading

Crazycolorz5 commented Feb 22, 2019 • edited Loading

StanHash commented Feb 22, 2019 • edited Loading

Crazycolorz5 commented Feb 22, 2019

StanHash commented Feb 22, 2019

Choose a reason for hiding this comment

Crazycolorz5 commented Feb 22, 2019

StanHash commented Feb 22, 2019 •

edited

Loading

StanHash commented Feb 22, 2019 •

edited

Loading

Crazycolorz5 commented Feb 22, 2019 •

edited

Loading

StanHash commented Feb 22, 2019 •

edited

Loading