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 Double and Int Convenience Properties #234
Changes from all commits
03c72f5
9dc4876
a2f349b
0206df1
4d17dd1
07d10b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
//===- SyntaxConvenienceMethods.swift - Convenience funcs for syntax nodes ===// | ||
// | ||
// This source file is part of the Swift.org open source project | ||
// | ||
// Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors | ||
// Licensed under Apache License v2.0 with Runtime Library Exception | ||
// | ||
// See https://swift.org/LICENSE.txt for license information | ||
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
public extension FloatLiteralExprSyntax { | ||
var floatingValue: Double { | ||
return potentialFloatingValue! | ||
} | ||
|
||
fileprivate var potentialFloatingValue: Double? { | ||
let floatingDigitsWithoutUnderscores = floatingDigits.text.filter { | ||
$0 != "_" | ||
} | ||
return Double(floatingDigitsWithoutUnderscores) | ||
} | ||
} | ||
|
||
public extension IntegerLiteralExprSyntax { | ||
var integerValue: Int { | ||
return potentialIntegerValue! | ||
} | ||
|
||
fileprivate var potentialIntegerValue: Int? { | ||
let text = digits.text | ||
let (prefixLength, radix) = IntegerLiteralExprSyntax.prefixLengthAndRadix(text: text) | ||
let digitsStartIndex = text.index(text.startIndex, offsetBy: prefixLength) | ||
let textWithoutPrefix = text.suffix(from: digitsStartIndex) | ||
|
||
let textWithoutPrefixOrUnderscores = textWithoutPrefix.filter { | ||
$0 != "_" | ||
} | ||
|
||
return Int(textWithoutPrefixOrUnderscores, radix: radix) | ||
} | ||
|
||
private static func prefixLengthAndRadix(text: String) -> (Int, Int) { | ||
let nonDecimalPrefixLength = 2 | ||
|
||
let binaryPrefix = "0b" | ||
let octalPrefix = "0o" | ||
let decimalPrefix = "" | ||
let hexadecimalPrefix = "0x" | ||
|
||
let binaryRadix = 2 | ||
let octalRadix = 8 | ||
let decimalRadix = 10 | ||
let hexadecimalRadix = 16 | ||
|
||
switch String(text.prefix(nonDecimalPrefixLength)) { | ||
case binaryPrefix: | ||
return (binaryPrefix.count, binaryRadix) | ||
case octalPrefix: | ||
return (octalPrefix.count, octalRadix) | ||
case hexadecimalPrefix: | ||
return (hexadecimalPrefix.count, hexadecimalRadix) | ||
default: | ||
return (decimalPrefix.count, decimalRadix) | ||
} | ||
} | ||
} | ||
|
||
public extension IntegerLiteralExprSyntax { | ||
var isValid: Bool { | ||
potentialIntegerValue != nil | ||
} | ||
} | ||
|
||
public extension FloatLiteralExprSyntax { | ||
var isValid: Bool { | ||
potentialFloatingValue != nil | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,7 +57,11 @@ public enum SyntaxFactory { | |
% param_type = param_type + "?" | ||
% child_params.append("%s: %s" % (child.swift_name, param_type)) | ||
% child_params = ', '.join(child_params) | ||
% if node.must_uphold_invariant: | ||
public static func make${node.syntax_kind}(${child_params}) -> ${node.name}? { | ||
% else: | ||
public static func make${node.syntax_kind}(${child_params}) -> ${node.name} { | ||
% end | ||
let layout: [RawSyntax?] = [ | ||
% for child in node.children: | ||
% if child.is_optional: | ||
|
@@ -82,7 +86,7 @@ public enum SyntaxFactory { | |
} | ||
% end | ||
|
||
% if not node.is_base(): | ||
% if not node.is_base() and not node.must_uphold_invariant: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically, I don’t think it’s correct to remove the In the future we might add invariants to other nodes as well that could have a valid blank content. One example that jumps to my mind would be string literals. For consistency, I would rather have this method return an Optional and return @akyrtzi What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to step back a bit here, what is the use case for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Paging @harlanhaskins for my question. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ping @harlanhaskins There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh sorry! I think |
||
public static func makeBlank${node.syntax_kind}() -> ${node.name} { | ||
let data = SyntaxData.forRoot(RawSyntax.create(kind: .${node.swift_syntax_kind}, | ||
layout: [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,7 +91,12 @@ open class SyntaxRewriter { | |
if let newNode = visitAny(node._syntaxNode) { return newNode } | ||
return Syntax(visit(node)) | ||
% else: | ||
% if node.must_uphold_invariant: | ||
// We know that the SyntaxData is valid since we are walking a valid syntax tree and haven't modified the syntax data. Thus the initializer below will never return nil. | ||
let node = ${node.name}(data)! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a comment like the following here to document why the use of the // We know that the SyntaxData is valid since we are walking a valid syntax tree and haven't modified the syntax data. Thus the below initialiser will never return nil. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you forgot to add the comment ;-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry about that. Fixed. |
||
% else: | ||
let node = ${node.name}(data) | ||
% end | ||
// Accessing _syntaxNode directly is faster than calling Syntax(node) | ||
visitPre(node._syntaxNode) | ||
defer { visitPost(node._syntaxNode) } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically,
_
at the start of each component is not valid. e.g._12
,0x_12
,0._12
and12e_3
. But I think it's OK to accept them for now.@akyrtzi What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine but adding a FIXME comment here would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rintaro I observe that
_12
and0x_2
don't parse asIntegerLiteralExprSyntax
orFloatLiteralExprSyntax
, respectively. My properties therefore shouldn't get those sorts of input. Should the properties nonethelessfatalError()
if the_
is the first character after the base prefix (or is the first character in the case of base 10)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @akyrtzi said in another comment, we should prevent a case of creating syntax node with invalid literal text. If you do that, you don't need to check
_
in thisintegerValue
orfloatingValue
accessor.Also, when you check them, you need to check not only for the integral part, but you also need to check the fractional part and the exponent part. That's not so simple to implement in this PR. So I suggest you to add a
FIXME
comment as @akyrtzi said.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that for now it’s sufficient if we uphold the invariant that the contents will of a integer / float literal are always parsable from the
integerValue
resp.floatingValue
properties, especially since the implementation is nice and short so far!That should already be an improvement over the current behaviour and if we add a FIXME, we can come back to it and improve it later.