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

remove unnecessary ccgutils dependency #1330

Merged
merged 3 commits into from
May 31, 2024

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented May 31, 2024

Summary

Remove an obsolete usage of ccgIntroducedPtr from sem, breaking an
import dependency between sem and the code generator, and allowing for
ccgIntroducedPtr to move back to ccgtypes.

Details

It was originally possible for the arguments to a copy hook overlapping
in memory (a parameter aliasing violation), resulting in incorrect
behaviour when copying variant objects if they do.

ed12679 worked around this by checking
whether both parameters were (hidden) pointers to the same location,
skipping the copy if they are, with ccgIntroducedPtr used to check
whether the source parameter is passed as via a pointer indirection.

This is a layering violation, and also no longer necessary, since
injecthooks ensures that the immutable argument doesn't refer to the
same memory location as the mutable one -- the workaround in
liftdestructors can thus be removed.

Copy hook injection makes sure that the operands both have a unique
address; the guard is thus obsolete, and with it the import.
The procedure was moved to `ccgutils` when the address equality guard
was added; now it's moved back into `ccgtypes` again.
@zerbina zerbina added compiler/backend Related to backend system of the compiler simplification Removal of the old, unused, unnecessary or un/under-specified language features. labels May 31, 2024
@zerbina zerbina added this to the C backend rework milestone May 31, 2024
@saem
Copy link
Collaborator

saem commented May 31, 2024

/merge

Copy link

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

  • a preparation for the MIR type IR; ccgIntroducedPtr being in ccgutils limits its implementation

@chore-runner chore-runner bot enabled auto-merge May 31, 2024 21:41
@chore-runner chore-runner bot added this pull request to the merge queue May 31, 2024
Merged via the queue into nim-works:devel with commit 3618e76 May 31, 2024
31 checks passed
@zerbina zerbina deleted the remove-ccgutils-import branch June 2, 2024 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/backend Related to backend system of the compiler simplification Removal of the old, unused, unnecessary or un/under-specified language features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants