defano / wyldcard

A clone of Apple's HyperCard and HyperTalk scripting language.
https://github.com/defano/wyldcard/wiki
MIT License
117 stars 12 forks source link

grammar: 'wait' and "default" timeUnit #67

Closed Jimw338 closed 5 years ago

Jimw338 commented 5 years ago

The "default" timeUnit for the wait command was seconds (HC 2.4.1 - What version is WyldCard "source-behavior" compatible with?). How would I implement such a "default argument" in the WyldCard grammar? ("WyldGram" ;) ) The appropriate part of the g4 file is:

| 'wait' expression timeUnit       # waitCountCmd

and the Java function:

public Object visitWaitCountCmd(HyperTalkParser.WaitCountCmdContext ctx) { return new WaitCmd(ctx, (Expression) visit(ctx.expression()), (TimeUnit) visit(ctx.timeUnit())); }

If I changed the grammar to:

| 'wait' expression timeUnit?       # waitCountCmd

(like with the "background? field") it would accept something like "wait 5". But then what will the "timeUnit" part of the context ("visit(ctx.timeUnit())) get set to (or is there then a timeUnit part of the context)? I suppose I could make another separate waitCountSeconds like

g4: | 'wait' expression # waitCountSecondsCmd

and then:

public Object visitWaitCountCmd(HyperTalkParser.WaitCountCmdContext ctx) { return new WaitSecondsCmd(ctx, (Expression) visit(ctx.expression())); }

and a separate waitSecondsCmd with: this.units = TimeUnit.SECONDS;

Or change the visitWaitCountCmd to (pseudocode - or HyperTalk): if (ctx IS EMPTY) then return new WaitSecondsCmd(ctx, TimeUnit.SECONDS);

(I guess this would be self-hosting ANTLR Hypercard Implementation in HyperTalk! Isn't it the "gold standard" of compiler construction that a language is able to self-host?)

Both of these seem inelegant - that the proper place for this logic is in the grammar, not the implementation code. Is there a "default argument syntax" like this:

| 'wait' expression timeUnit?=seconds       # waitCountCmd

or:

| 'wait' expression timeUnit?=secondsTimeUnit()       # waitCountCmd

so that the "timeUnits" node of the AST context is automatically set to "secondsTimeUnit"? I'm definitely putting in the ordering Terrance Parr's "Definitive ANTLR 4 Reference" TODAY :). I found a pirated PDF file, but I just like books!

What is your convention for capitalization? I notice that the "visit" functions and "helper evaluators" like secondsTimeUnit() are lower case, while the "Cmd" funcitons are upper-case.

defano commented 5 years ago

Good catch. In general, I'm using HC 2.4.1 as my reference but I'm sure there are dozens and dozens of similar "incompatibilities". Thanks for pointing these out.

I would fix this problem by simply changing the timeUnit rule so that it looks like this:

timeUnit
    : 'ticks'       # ticksTimeUnit
    | 'tick'        # tickTimeUnit
    | seconds       # secondsTimeUnit
    |               # secondsTimeUnit
    ;

That last rule (the empty |) simply says that when no time unit is present, fire the secondsTimeUnit rule in the tree visitor thus producing a TimeUnit.SECONDS object in the AST.

Alternately, you could add a ? after timeUnit in the wait command rules, but then, as you point out, you'll have to deal with a missing time unit value in either the tree visitor or the WaitCmd class.

I don't think Antlr supports a default syntax, but I could be wrong.

Lastly, I'm not sure I understand your capitalization question... sounds like you're asking why visitWaitCountCmd starts with a lowercase but WaitCmd starts with an uppercase. If so, that's basic Java convention: Methods always start with a lowercase, but class names start with an uppercase.

Happy to let you send me a pull request with that change if you'd like "credit"--if not, easy enough for me to make the change directly.

Jimw338 commented 5 years ago

I don't think Antlr supports a default syntax, but I could be wrong. Ah, maybe I could suggest it for ANTLR 4.4.. :). I didn't know about the Java convention - or didn't notice that WaitCmd was a class (rather than a command being called or something). It makes sense.

You can make the change - time to get a Git book too