Skip to content

Conversation

ghost
Copy link

@ghost ghost commented May 10, 2018

Similar to #24028

@ghost ghost requested a review from amcasey May 10, 2018 21:31
function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, start: number): void {
const token = getTokenAtPosition(sourceFile, start, /*includeJsDocComment*/ false);
const statement = cast(token.parent, isLabeledStatement).statement;
changes.deleteRange(sourceFile, { pos: token.getStart(sourceFile), end: statement.getStart(sourceFile) });
Copy link
Contributor

Choose a reason for hiding this comment

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

this will remove comments on the statement before the label.. if you are deleting a range, why not just delete from token.pos to :.end?

Copy link
Author

Choose a reason for hiding this comment

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

I wanted to remove the space between the : and the statement following it. token.getStart should not include any comments preceding token. (But token.pos would.)

Copy link
Member

Choose a reason for hiding this comment

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

/*not deleted*/ label /*deleted*/ : /*deleted*/ call();?

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

I have concerns about the trivia, but I understand you're tracking that as its own (increasingly expensive) work item. Otherwise, LGTM.

function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, start: number): void {
const token = getTokenAtPosition(sourceFile, start, /*includeJsDocComment*/ false);
const statement = cast(token.parent, isLabeledStatement).statement;
changes.deleteRange(sourceFile, { pos: token.getStart(sourceFile), end: statement.getStart(sourceFile) });
Copy link
Member

Choose a reason for hiding this comment

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

/*not deleted*/ label /*deleted*/ : /*deleted*/ call();?


// @noUnusedLocals: true

////label1: while (1) {}
Copy link
Member

Choose a reason for hiding this comment

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

What happens for outdented labels on preceding lines?

label:
    code

Does formatting restore the indentation? Otherwise, it seems like you'd end up with code outdented.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@ghost ghost force-pushed the fixUnusedLabel branch from 3a501cc to 3e1f013 Compare May 18, 2018 21:43
@ghost ghost force-pushed the fixUnusedLabel branch from 3e1f013 to 76f56a5 Compare May 18, 2018 22:05
@ghost ghost merged commit 3eb66da into master May 18, 2018
@ghost ghost deleted the fixUnusedLabel branch May 18, 2018 22:25
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants