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

Some small cleanups #4331

Closed
wants to merge 7 commits into from
Closed

Some small cleanups #4331

wants to merge 7 commits into from

Conversation

Dante-Broggi
Copy link
Contributor

I should actually PR the cleanup and improvements I find.

this matches lists and hashmaps
Although def->permissions is only NULL on non-actor behaviors and type alias functions,
And thus they should never get here, we should assert before we null deref
Instead cast at each use with the new type.
with the other HASHMAP functions by taking a name_t, not a general hashmap_t
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Mar 20, 2023
@SeanTAllen
Copy link
Member

Please break these into logical commits with a PR for each.

If these ever come up in history as "why was this changed", bunching them altogether will be detrimental to understanding.

Additionally, it's not guaranteed that we will want to accept each of these changes.

@SeanTAllen
Copy link
Member

Some of the changes I'm good with, others I'm not.

@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label Mar 20, 2023
@Dante-Broggi
Copy link
Contributor Author

Ok, I'll make a PR for each commit, they are all mutually separable.

@SeanTAllen
Copy link
Member

Thanks @Dante-Broggi. I'm closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge This PR should not be merged at this time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants