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

TY: convert primitive types from object to class #7503

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

Kobzol
Copy link
Member

@Kobzol Kobzol commented Jul 11, 2021

This is a naive attempt to reduce the usage of object in primitive types and convert them to standard classes instead. This is probably needed as a first step to solve #5080. I didn't change TyNever and TyUnknown, because we probably don't need aliases to work for them (?).

The ultimate plan is to move aliasedBy from TyAdt to Ty. Which will probably introduce all kinds of scary breakage and problems (especially the description of this PR #4243 scares me in relation to comparing types :D ).

Sending PR to see what kinds of tests fail.

@Kobzol Kobzol requested a review from vlad20012 July 11, 2021 15:40
@Kobzol Kobzol added the internal Pull requests about internal improvements/fixes that don't affect users directly label Jul 11, 2021
Copy link
Member

@vlad20012 vlad20012 left a comment

Choose a reason for hiding this comment

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

LGTM.

The one thing I'd made additionally is turning name and ordinal fields in TyInteger and TyFloat classes into abstract properties with getters overridden in descendant classes.

Which will probably introduce all kinds of scary breakage and problems (especially the description of this PR #4243 scares me in relation to comparing types

Well, == just shouldn't be used for Tys. But there are already quite lot of such usages and I have no idea how to find them

I didn't change TyNever and TyUnknown, because we probably don't need aliases to work for them (?).

Most likely yes.

@Kobzol Kobzol force-pushed the ty-reduce-singleton-usage branch from a48ee79 to cc28619 Compare July 15, 2021 08:02
@Kobzol
Copy link
Member Author

Kobzol commented Jul 15, 2021

Good idea, I changed them.

EDIT: Oh, I thought that override val name: String = "foo" will just generate a getter without a field, but it does not seem to be the case :( I'll convert them to actual getters.

@Kobzol Kobzol force-pushed the ty-reduce-singleton-usage branch from cc28619 to 08205f5 Compare July 15, 2021 08:13
@Kobzol
Copy link
Member Author

Kobzol commented Jul 15, 2021

Now all primitive types implement these properties with getters and there should be no memory wasted on backing fields.

@Kobzol
Copy link
Member Author

Kobzol commented Jul 15, 2021

bors r=vlad20012

bors bot added a commit that referenced this pull request Jul 15, 2021
7503: TY: convert primitive types from object to class r=vlad20012 a=Kobzol

This is a naive attempt to reduce the usage of `object` in primitive types and convert them to standard classes instead. This is probably needed as a first step to solve #5080. I didn't change `TyNever` and `TyUnknown`, because we probably don't need aliases to work for them (?).

The ultimate plan is to move `aliasedBy` from `TyAdt` to `Ty`. Which will probably introduce all kinds of scary breakage and problems (especially the description of this PR #4243 scares me in relation to comparing types :D ).

Sending PR to see what kinds of tests fail.

Co-authored-by: Jakub Beránek <berykubik@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jul 15, 2021

Build failed:

@Kobzol
Copy link
Member Author

Kobzol commented Jul 15, 2021

bors retry

bors bot added a commit that referenced this pull request Jul 15, 2021
7503: TY: convert primitive types from object to class r=vlad20012 a=Kobzol

This is a naive attempt to reduce the usage of `object` in primitive types and convert them to standard classes instead. This is probably needed as a first step to solve #5080. I didn't change `TyNever` and `TyUnknown`, because we probably don't need aliases to work for them (?).

The ultimate plan is to move `aliasedBy` from `TyAdt` to `Ty`. Which will probably introduce all kinds of scary breakage and problems (especially the description of this PR #4243 scares me in relation to comparing types :D ).

Sending PR to see what kinds of tests fail.

Co-authored-by: Jakub Beránek <berykubik@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jul 15, 2021

Build failed:

@Kobzol
Copy link
Member Author

Kobzol commented Jul 15, 2021

bors retry

bors bot added a commit that referenced this pull request Jul 15, 2021
7503: TY: convert primitive types from object to class r=vlad20012 a=Kobzol

This is a naive attempt to reduce the usage of `object` in primitive types and convert them to standard classes instead. This is probably needed as a first step to solve #5080. I didn't change `TyNever` and `TyUnknown`, because we probably don't need aliases to work for them (?).

The ultimate plan is to move `aliasedBy` from `TyAdt` to `Ty`. Which will probably introduce all kinds of scary breakage and problems (especially the description of this PR #4243 scares me in relation to comparing types :D ).

Sending PR to see what kinds of tests fail.

Co-authored-by: Jakub Beránek <berykubik@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jul 15, 2021

Build failed:

@Kobzol
Copy link
Member Author

Kobzol commented Jul 15, 2021

bors retry

@bors
Copy link
Contributor

bors bot commented Jul 15, 2021

Merge conflict.

@Kobzol Kobzol force-pushed the ty-reduce-singleton-usage branch from 08205f5 to 92bc5cf Compare July 15, 2021 15:24
@Kobzol
Copy link
Member Author

Kobzol commented Jul 15, 2021

bors r=vlad20012

@bors
Copy link
Contributor

bors bot commented Jul 15, 2021

Build succeeded:

@bors bors bot merged commit 6817f4d into intellij-rust:master Jul 15, 2021
@github-actions github-actions bot added this to the v152 milestone Jul 15, 2021
@Kobzol Kobzol deleted the ty-reduce-singleton-usage branch July 15, 2021 22:02
@vlad20012 vlad20012 self-assigned this Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Pull requests about internal improvements/fixes that don't affect users directly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants