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

Format pointers to have asterisk next to type instead of identifier #49875

Closed
wants to merge 2 commits into from

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Dec 9, 2020

Fixes #49733

@Youssef1313 Youssef1313 changed the title draft Format pointers to have asterisk next to type instead of identifier Dec 10, 2020
Comment on lines 511 to 517
if (currentToken.Kind() == SyntaxKind.AsteriskToken && currentToken.Parent is PointerTypeSyntax)
{
return CreateAdjustSpacesOperation(1, AdjustSpacesOption.ForceSpacesIfOnSingleLine);
}

if (previousToken.Kind() == SyntaxKind.AsteriskToken &&
(previousToken.Parent is PrefixUnaryExpressionSyntax || previousToken.Kind() == SyntaxKind.IdentifierToken))
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE: The original issue complains about NormalizeWhitespace, I'm not sure whether this affects NormalizeWhitespace or not.

@Youssef1313 Youssef1313 marked this pull request as ready for review December 10, 2020 11:39
@Youssef1313 Youssef1313 requested a review from a team as a code owner December 10, 2020 11:39
@Youssef1313
Copy link
Member Author

The number of failing tests is large. So I'd like to have a review on the code change before fixing the tests.

Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

This implementation change affects the formatter, which is completely unrelated code from NormalizeWhitespace.

(previousToken.Kind() == SyntaxKind.AsteriskToken && previousToken.Parent is PrefixUnaryExpressionSyntax))
if (currentToken.Kind() == SyntaxKind.AsteriskToken && currentToken.Parent is PointerTypeSyntax)
{
return CreateAdjustSpacesOperation(1, AdjustSpacesOption.ForceSpacesIfOnSingleLine);
Copy link
Member

Choose a reason for hiding this comment

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

❗ I believe this will break the formatter for types like int** (it will start to format them as int* *.

Copy link
Member Author

@Youssef1313 Youssef1313 Dec 11, 2020

Choose a reason for hiding this comment

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

@sharwell Do we want to fix both the formatter and NormalizeWhitespace? or only NormalizeWhitespace?

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we always adjust this to 0 spaces if we have the * of a pointer type? i would think we would always want zero spaces between that and whatever comes before it

Copy link
Member Author

Choose a reason for hiding this comment

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

@CyrusNajmabadi Whops, I mixed things up. Swapped the expected behavior and the actual behavior.
The formatter behaves as expected. This is only a NormalizeWhitespace issue.
Closing the PR.

@jinujoseph jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 10, 2020
@jinujoseph jinujoseph added this to InQueue in IDE: CommunityPR via automation Dec 10, 2020
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Change seems wrong. also, missing tests.

IDE: CommunityPR automation moved this from InQueue to Completed Dec 11, 2020
@Youssef1313 Youssef1313 deleted the patch-5 branch December 11, 2020 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
IDE: CommunityPR
  
Completed 2020
Development

Successfully merging this pull request may close these issues.

NormalizeWhitespace should put * next to the type rather than the identifier
5 participants