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

[decompiler] rewrite set lets as just sets #1858

Merged
merged 5 commits into from
Sep 7, 2022
Merged

Conversation

ManDude
Copy link
Member

@ManDude ManDude commented Sep 7, 2022

Rewrites specific kinds of lets where the return value of the set! itself is meant to be used to just be set!s. Implemented at the let rewrite level. Seems to work for Jak 2 so far.

Fixes #1854 .

@ManDude ManDude changed the title D/set let proto [decompiler] rewrite set lets as just sets Sep 7, 2022
* dest-var
* )
* to:
* (set! something src)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this change is unsafe to make - for example:

(let ((temp (foo)))
  (set! (-> (bar) field) temp)
  )

will call foo before bar, but

(set! (-> (bar) field) (foo))

will call bar before foo.

Which is why I think this detection has to happen before expression building.
If we do it here, we also lose the ability to detect things like *!

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering that too, but it looks like set! is an exception to the left-to-right eval order. The source is eval'd before the destination.
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Math macros like *! also seemed unaffected, looking at the refs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah it looks like you're right - both the compiler and decompiler agree about this, and I made a comment about it being flipped here
image

I think that means this is safe, but I do want to think about it a little more.

The *! thing would only apply in cases where we don't detect *! currently - if you had something that was originally (*! foo bar), but it got split up due to this set/let issue, then this fix will give us (set! foo (* foo bar)). Which I wouldn't worry about too much.

A few other things I want to think about

  • uint128/vf stuff : seems like it doesn't get applied for vf stuff because that doesn't become a set
  • dropping casts for float/int convers: I think it won't but I want to review some more
  • dropping casts for (the-as - not sure, I kinda think this will be wrong
  • bitfield stuff: the compile makes set! on a bitfield return none, but we could change this

@ManDude ManDude marked this pull request as ready for review September 7, 2022 02:45
@ManDude
Copy link
Member Author

ManDude commented Sep 7, 2022

Only problem might be that it won't cast things like enums or time-frame properly (see seagull). Hasn't caused actual issues so far though so I won't spend time fixing it right now.

@water111 water111 merged commit cfb6abb into master Sep 7, 2022
@water111 water111 deleted the d/set-let-proto branch September 7, 2022 22: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.

[decompiler] (set! foo bar) bar could be cleaner
2 participants