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

Improve line-break detection to handle "/*" inside "//" comments, #2129

Merged
merged 2 commits into from
Apr 18, 2018

Conversation

lukastaegert
Copy link
Member

Resolves #2127

Previously, line-break detection outside comments was faulty in that since line comments cannot contain line-breaks, it would only scan the code for block comments. This approach failed if a block comment start /* was nested inside a line comment.

The new logic now scans for all types of comments and ignores any markup inside both types of comments.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Nice work on the quick fix here.

do {
commentStart = code.indexOf('/', start);
if (commentStart === -1 || commentStart > lineBreakPos) return lineBreakPos;
nextChar = code[commentStart + 1];
Copy link
Contributor

Choose a reason for hiding this comment

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

charCodeAt perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course! Has been changed. I also slightly improved it to not misinterpret /*/ as an immediately closed block comment

@@ -45,17 +45,20 @@ export function findFirstOccurrenceOutsideComment(
}

function findFirstLineBreakOutsideComment(code: string, start: number = 0) {
let commentStart, lineBreakPos;
let commentStart, lineBreakPos, nextChar;
Copy link
Contributor

Choose a reason for hiding this comment

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

I take it the assumption in this function is that start is always at the end of all the code for a line?

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens in cases like:

(code) /*

*/ (moar code)

Will this be able to deal with the start of a code block inline with the block comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is actually no assumption about start. Basically we

  • find the next line-break following start
  • find the next comment following start if it exists and see if it closes before we reach the line-break
  • if there is a block comment, we find the end of the comment and repeat with the next line-break outside the comment

Thus in the situation above, the first line-break will be ignored and since there is not line-break between the end of the comment and the next statement, the algorithm will return -1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh so the code passed to this function in my example above would only ever be the " /*\n\n*/ " part, that is it is guaranteed to be a substring between AST Nodes already?

@lukastaegert lukastaegert force-pushed the ignore-comment-start-in-comment branch from 6a12566 to 1872d80 Compare April 18, 2018 05:35
@lukastaegert
Copy link
Member Author

Also adjusted findFirstOccurrenceOutsideComment to work more similarly to findFirstLineBreakOutsideComment and require less updating the search position.

@lukastaegert lukastaegert force-pushed the ignore-comment-start-in-comment branch from 1872d80 to 0391890 Compare April 18, 2018 07:03
@guybedford guybedford merged commit 603f527 into master Apr 18, 2018
@lukastaegert
Copy link
Member Author

Released as 0.58.1.

@lukastaegert lukastaegert added this to the 0.58.1 milestone Apr 18, 2018
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.

None yet

2 participants