Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reduce/reduce conflicts and precedence fixes #64

Merged
merged 8 commits into from
Nov 7, 2019
Merged

reduce/reduce conflicts and precedence fixes #64

merged 8 commits into from
Nov 7, 2019

Conversation

DankRank
Copy link
Member

@DankRank DankRank commented Nov 1, 2019

The only remaining r/r conflicts are in the following places:
IDENTIFIER "(" Instruction_Parameter • "," Instruction_Parameter • ")" • ";"

  • The first two are because Instruction_Parameter includes Expression, and both of them include Load_Type.
  • The last one is because Instruction includes Expression, and both of them include IDENTIFIER"(" Instruction_Parameters ")"
    Both of these are kinda hard to fix.

This fixes 6 conflicts and creates 2. To completely fix it,
Instruction_Parameter should rely on Expression rule, instead of using
Load_Type directly. I'm not sure what are the implications of this
though.
The Expression rule in Inst_Param makes it redundant.
It was introduced in 348d4f3
Instructions already contains %empty.
The empty rule was added in 8152e3d
It might be a typo though??? Priw8 pls check.
now it's only used for precedence, like it should be.

- remove th10_alternatives, since it's unused.
this doesn't change the output of bison.

Also, if they _did_ have associativity, SIN/COS/SQRT would be
right-associative.
Left assoc:  SIN COS x -> SIN (COS x)
Right assoc: SIN COS x -> (SIN COS) x
"each rule gets its precedence from the last terminal symbol mentioned
in the components"
 - (c) gnu bison manual

I could change the thing in precedence declarations to ":", but that
would probably affect other places where ":" is used.

This commit doesn't change the output of bison.
However, I tried replacing Expression_Safe with Expression in the ?:
rule and there is a difference: 17 s/r conflicts fixed.
@DankRank DankRank requested a review from Priw8 November 1, 2019 04:51
Copy link
Member

@Priw8 Priw8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty rule was added in 8152e3d
It might be a typo though??? Priw8 pls check.

This was intentional, without the empty rule subs without any instructions would be dumped fine, but trying to recompile them would result in an error. I didn't realize that CaseList also had an empty rule, so I accidentaly left it in.


This fixes 6 conflicts and creates 2. To completely fix it, Instruction_Parameter should rely on Expression rule, instead of using Load_Type directly. I'm not sure what are the implications of this though.

I believe just changing it to Expression would result in every instruction parameter being passed on the stack (and also completely break pre-th10 ECL which has no stack). One solution to this would be making Instruction_Parameter automatically convert params that reference EXPRESSION_VAL type expressions to params with just the value from the expression (and remove the expression, obviously). This could only be done to uncasted expressions though (with value->type being the same as result_type), as casted expressions have to be passed on the stack in order to make the game typecast them. Something similar to this (converting expression params to non-expression params) actually exists for sub calls by name (to interpret _f(10) as _fS 10)


The last one is because Instruction includes Expression, and both of them include IDENTIFIER"(" Instruction_Parameters ")"

I guess the only way of fixing that would be making all instruction/sub calls created as expressions? That doesn't seem simple to do though, since we need to know when to push the sub call return values... It should be possible to tell that based on whether the call expression has any parent expressions, maybe?
someCall(1, 2); <- no parent expressions, regular call
someCall(1, 2) + 2; <- parent expression is the ADD operation, need to output return value
There is 1 more issue though: the result_type of the call expression needs to be set when the expression is created, not when it's outputted. This is not a problem if the called sub is defined before the expression, but when it's not, the result type is unknown. Right now, called subs do not have to be declared before the call if they are not a part of an expression, as their return type is irrelevant, which makes them not affected by this limitation. However, making all calls be created as expressions would result in regular calls having the same limitations as calls that need the return value. There is no easy solution to that, as it's impossible to know whether the return type will be needed when the call expression is created. While it could just default to void if the called sub is not found, this would result in error messages not actually telling what the issue is in some cases (I think it would throw the "no expression found for type(s)" error when a sub with a needed return value is not found). The real solution to that would be some sort of pre-parsing of the input that would create a list of all subs and their return values. This would remove the need of subs being declared before they can be used as expressions.
...or, we could just require all subs to be declared before being called by name, but I don't think we should.

@DankRank
Copy link
Member Author

DankRank commented Nov 1, 2019

I believe just changing it to Expression would result in every instruction parameter being passed on the stack.

Correct. I was thinking about fixing it by adding a flag to expr, that gets set for the Load_Type expressions, and unset for any other expressions. This way one could still produce degenerate LOAD opcodes with grouping expression (i.e. (1)), and the decompiler would need to be tweaked to wrap those in (). Also, having thecl06 not crash when an expression is encountered would be nice. It would probably require delaying the symbol (LOADI/LOADF) resolution.

I guess the only way of fixing that would be making all instruction/sub calls created as expressions?

ez fix would be to have the scanner determine if identifier is a instruction name by looking it up in the eclmap. I'll probably do it after #63 and this are merged.

@DankRank DankRank merged commit d83eb1f into master Nov 7, 2019
@DankRank DankRank deleted the rrfix branch November 7, 2019 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants