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.

Sign up for GitHub

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

Jump to bottom

Introduce a concept of internal special types, move all members defined in special types to the SpecialMember set. #72646

Merged
merged 4 commits into from
Mar 22, 2024
Merged

Introduce a concept of internal special types, move all members defined in special types to the SpecialMember set. #72646

merged 4 commits into from
Mar 22, 2024

Conversation

AlekseyTs
Copy link
Contributor

Closes #72102.
Closes #72280.

@AlekseyTs
Introduce a concept of internal special types, move all members defin…
830bdd2 
…ed in special types to the SpecialMember set.

Closes dotnet#72102.
Closes dotnet#72280.
@AlekseyTs AlekseyTs added Concept-API This issue involves adding, removing, clarification, or modification of an API. Area-Compilers labels Mar 21, 2024
@AlekseyTs AlekseyTs requested review from a team as code owners March 21, 2024 18:47
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Mar 21, 2024
@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review

333fred
333fred reviewed Mar 21, 2024
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

A few small comments. Overall this looks good.

src/Compilers/Core/Portable/ExtendedSpecialType.cs

public override string ToString()
{
return _value.ToString();
Copy link
Member

@333fred 333fred Mar 21, 2024 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

I'm wondering whether it would be useful, for debugging purposes, to instead have ToString determine whether _value is a special type or an internal special type, then call ToString on the appropriate value, so we get the right string representation. I think I'd at least like a DebuggerDisplay with that logic. #Pending

src/Compilers/Core/Portable/InternalSpecialType.cs Outdated Show resolved Hide resolved
src/Compilers/Core/Portable/MemberDescriptor.cs Outdated Show resolved Hide resolved
@AlekseyTs @333fred
Apply suggestions from code review
c0f4f12 
Co-authored-by: Fred Silberberg <fred@silberberg.xyz>
333fred
333fred approved these changes Mar 21, 2024
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Talked with Aleksey about following up on the ToString change separately.

@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler For the second review.

jcouv
jcouv reviewed Mar 21, 2024
View reviewed changes
src/Compilers/Core/Portable/ExtendedSpecialType.cs Outdated
public static explicit operator ExtendedSpecialType(int value) => new ExtendedSpecialType(value);
public static explicit operator int(ExtendedSpecialType value) => value._value;

public static bool operator ==(ExtendedSpecialType left, SpecialType right) => left._value == (byte)right;
Copy link
Member

@jcouv jcouv Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why do we need multiple ==/!= operators? The (ExtendedSpecialType, ExtendedSpecialType) flavor should be sufficient given the SpecialType -> ExtendedSpecialType implicit conversion. #Closed

@jcouv jcouv self-assigned this Mar 21, 2024
jcouv
jcouv reviewed Mar 21, 2024
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 2)

@AlekseyTs AlekseyTs requested a review from jcouv March 22, 2024 03:57
jcouv
jcouv approved these changes Mar 22, 2024
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 3)

@AlekseyTs AlekseyTs enabled auto-merge (squash) March 22, 2024 04:09
@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Mar 22, 2024 •
edited
Loading 

        Dim getTypeMethod = DirectCast(Me._module.Compilation.GetWellKnownTypeMember(WellKnownMember.System_Type__GetTypeFromHandle), MethodSymbol)

I am going to add this symbol to the BoundGetType in a follow-up so that it always looked up in the result type of the node. #Pending

Refers to: src/Compilers/VisualBasic/Portable/CodeGen/EmitExpression.vb:2230 in 6eda453. [](commit_id = 6eda453, deletion_comment = False)

@AlekseyTs AlekseyTs merged commit 67ef1ee into dotnet:main Mar 22, 2024
24 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Mar 22, 2024
AlekseyTs added a commit to AlekseyTs/roslyn that referenced this pull request Mar 25, 2024
@AlekseyTs
Follow up on addition of InternalSpecialType
ff514df 
Related to dotnet#72646.
@AlekseyTs AlekseyTs mentioned this pull request Mar 25, 2024
Merged
AlekseyTs added a commit that referenced this pull request Mar 25, 2024
@AlekseyTs
Follow up on addition of InternalSpecialType (#72704)
48812dd 
Related to #72646.
@RikkiGibson RikkiGibson modified the milestones: Next, 17.10 P3 Mar 25, 2024
@dotnet-bot dotnet-bot mentioned this pull request Mar 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcouv jcouv jcouv approved these changes

@333fred 333fred 333fred approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.10 P3
4 participants
@AlekseyTs @333fred @jcouv @RikkiGibson