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

TreeOps: classify Type.Apply only as isCallSite #2749

Merged
merged 1 commit into from
Sep 21, 2021

Conversation

kitbellew
Copy link
Collaborator

It was also present in isDefnSite.

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

I wouldn't change this currently as it seems it could influence a lot of people's codebases and cause quite difficult to explain changes.

It was also present in isDefnSite.
Copy link
Collaborator Author

@kitbellew kitbellew left a comment

Choose a reason for hiding this comment

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

as you can see from the tests, the only place where this change affects the result is when binpacking is enabled. however, several changes we've merged over the last week (including #2751 an hour ago) have already changed how binpacking behaves, so by comparison this is small potatoes.

@@ -399,9 +399,9 @@ object TreeOps {
tree match {
case _: Decl.Def | _: Defn.Def | _: Defn.Macro | _: Defn.Class |
_: Defn.Trait | _: Ctor.Secondary | _: Decl.Type | _: Defn.Type |
_: Type.Apply | _: Type.Param | _: Type.Tuple | _: Defn.Enum |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

take a look at line 440 below. Type.Apply is present in both call site and defn site, but it can't be both.

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks for the explnations! LGTM

@kitbellew kitbellew merged commit adb236e into scalameta:master Sep 21, 2021
@kitbellew kitbellew deleted the 2748 branch September 21, 2021 13:15
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