[Driver] - Tokenization for response files #16
Conversation
ADD - tokenizeResponseFile(_:) and tokenizeResponseFileLine(_:) methods.
There was a problem hiding this comment.
Thanks, @JhonnyBillM, this looks great! Some small comments, but overall this is good.
| /// | ||
| /// - Complexity: O(*n*), where *n* is the length of the line. | ||
| private static func tokenizeResponseFileLine<S: StringProtocol>(_ line: S) -> String { | ||
| if line.isEmpty { return "" } |
There was a problem hiding this comment.
Can you return an optional here?
There was a problem hiding this comment.
Done.
Very good suggestion. Thanks!
| return content | ||
| .split(separator: "\n") | ||
| .map { tokenizeResponseFileLine($0) } | ||
| .filter { !$0.isEmpty } |
There was a problem hiding this comment.
And turn this map-then-filter into a compactMap?
…stead of empty lines.
| if line.isEmpty { return nil } | ||
|
|
||
| // Support double dash comments only if they start at the beginning of a line. | ||
| if line.first == "/", line.count > 1, line[line.index(after: line.startIndex)] == "/" { return nil } |
There was a problem hiding this comment.
Inverted the validation order to prevent doing a count operation if there is no chance to have a comment.
There was a problem hiding this comment.
This might be best as line.hasPrefix(“//“)
There was a problem hiding this comment.
🤯 Wonderful. Done.
|
Thanks for the review @harlanhaskins |
| /// - Parameter content: response file's content to be tokenized. | ||
| private static func tokenizeResponseFile(_ content: String) -> [String] { | ||
| return content | ||
| .split(separator: "\n").map { $0 } |
There was a problem hiding this comment.
Hm, does the result of Split not have a compactMap implementation?
There was a problem hiding this comment.
Removed.
Sorry, my bad! That was not supposed to be there and I entirely missed it out.
Thanks for calling that out.
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
Thanks! @DougGregor @harlanhaskins Can I check this bullet point in the README ? |
Merge pull request apple#16 from JhonnyBillM/tokenize-response-files
|
Feel free to submit another PR for that, if you want the credit for the commit 😄 |
This PR tries to address the "Implement proper tokenization for response files" to-do item that is on the README.
Now we can correctly parse:
This matches the current behavior of the LLVM tokenization method (the Swift compiler is currently using the LLVM method).
Also, after reading LLVM's ExpandResponseFiles method, I think our current implementation has all we need.
Note: we need to update this implementation once we get Windows support.
For "documentation" purposes
What are
Response files? Files that contains a list of command line arguments.For more info search for "response files" in this document