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

Add code fix to remove unused label #24037

Merged
3 commits merged into from
May 18, 2018
Merged

Add code fix to remove unused label #24037

3 commits merged into from
May 18, 2018

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 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
2 participants