Skip to content

sdfmt: Explicitly parse shortened function syntax - #380

Open
maxhaton wants to merge 1 commit into
snazzy-d:masterfrom
maxhaton:shortenedfn
Open

maxhaton wants to merge 1 commit into
snazzy-d:masterfrom
maxhaton:shortenedfn

Conversation

@maxhaton

@maxhaton maxhaton commented Sep 1, 2024

Copy link
Copy Markdown
Collaborator

This goes some way to Fix #359, but the result could probably be improved somewhat with a clever span somewhere.

This feature is presumably only used for things with fairly short names so one imagines the line wrapping behaviour won't be noticed much anyway...

@maxhaton
maxhaton requested a review from deadalnix September 1, 2024 22:55
Comment thread test/format/shortfnsyntax.d
Comment thread src/format/parser.d
Comment thread src/format/parser.d Outdated
}

void parseShortenedFunctionBody() in(match(TokenType.FatArrow)) {
auto spanGuard = span!Span();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do you not splice it?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I changed it, not entirely sure if I know what the difference is precisely?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Dump the IR, but long story short, the span is extended leftward in a way that fits the hierarchy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, if there is already a span that does that, then maybe it's not useful to add one more, but I don't remember what is the generated IR here.

Comment thread src/format/parser.d Outdated
Comment thread src/format/parser.d Outdated
Comment thread src/format/parser.d Outdated
space();
parseExpression();
split();
nextToken();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You might want to check it is the token you expect.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

What should it do if it's not? Nothing, then return?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, pretty much.

Comment on lines +3 to +4
int _______________________(int x)
=> x + 1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's good. We also need a test case where the expression in there splits.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added something that breaks.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Looks ok.

@deadalnix deadalnix Sep 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, this looks good. One thing I have though of since is that we maybe break after the type, in which case I think we do not indent. The way the span are laid out might break this, so it's a good idea to check.

Comment thread src/format/parser.d
nextToken();
space();
parseExpression();
if (match(TokenType.Semicolon)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is a function that does that.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

where?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Look for runOnType .

Comment thread src/format/parser.d Outdated
This goes some way to Fix snazzy-d#359, but the result could probably be
improved somewhat with a clever span somewhere.

This feature is presumably only used for things with fairly short names
so one imagines the line wrapping behaviour won't be noticed much
anyway...
Comment thread src/format/parser.d
}

void parseShortenedFunctionBody() in(match(TokenType.FatArrow)) {
auto spanGuard = spliceSpan();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it change something if you remove this? There might be a span here already, but if there isn't this is good to have.

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.

Formatting of functions using => instead of return

2 participants