Skip to content

Pass length of source file to C parsing actions #261

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

Merged
merged 1 commit into from
Mar 5, 2021

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Mar 4, 2021

Previously, the string was implicitly terminated by the first null character. But a source file might contain a null character which shouldn't terminate it.

@ahoppen
Copy link
Member Author

ahoppen commented Mar 4, 2021

swiftlang/swift#36283

@swift-ci Please test

@ahoppen ahoppen requested a review from akyrtzi March 4, 2021 17:21
@@ -192,7 +192,7 @@ public enum SyntaxParser {
swiftparse_parser_set_diagnostic_handler(c_parser, diagHandler)
}

let c_top = swiftparse_parse_string(c_parser, source)
let c_top = swiftparse_parse_string(c_parser, source, source.utf8.count)
Copy link
Member

@rintaro rintaro Mar 4, 2021

Choose a reason for hiding this comment

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

I believe passing String to const char * calls String.utf8CString which always copies the buffer. Probably

let c_top = source.withUTF8 {
  $0.withMemoryRebound(to: CChar.self) { buf in
    swiftparse_parse_string(c_parser, buf.baseAddress, buf.count)
  }
}

is faster unless it needs to be null terminated.

Copy link
Member Author

Choose a reason for hiding this comment

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

It needs to be null-terminated, otherwise lvm::MemoryBuffer::getMemBuffer is not happy (also we need the terminating null char to lex it as the EOF token).

Copy link
Member

@rintaro rintaro Mar 4, 2021

Choose a reason for hiding this comment

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

OK, in that case, I suggest

let c_top = source.withCString { buf in
  swiftparse_parse_string(c_parser, buf, source.utf8.count)
}

withCString doesn't copy the buffer if the underlying string buffer is null terminated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good 👍

Previously, the string was implicitly terminated by the first null
character. But a source file might contain a null character which
shouldn't terminate it.
@ahoppen ahoppen force-pushed the pr/null-character-in-file branch from af5448b to cb72454 Compare March 4, 2021 21:41
@ahoppen
Copy link
Member Author

ahoppen commented Mar 4, 2021

Updated to use withCString as @rintaro suggested. Nice catch on performance.

swiftlang/swift#36283

@swift-ci Please test

@swiftlang swiftlang deleted a comment from swift-ci Mar 4, 2021
@swiftlang swiftlang deleted a comment from swift-ci Mar 4, 2021
@ahoppen ahoppen merged commit 843c82c into swiftlang:main Mar 5, 2021
@ahoppen ahoppen deleted the pr/null-character-in-file branch January 14, 2023 08:21
adevress pushed a commit to adevress/swift-syntax that referenced this pull request Jan 14, 2024
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.

3 participants