[webkit-reviews] review denied: [Bug 91690] JSC: bug fixes and enhancements for OfflineASM annotation system : [Attachment 153132] Fix.

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 91690: JSC: bug fixes and enhancements for OfflineASM annotation system
https://bugs.webkit.org/show_bug.cgi?id=91690

Attachment 153132: Fix.
https://bugs.webkit.org/attachment.cgi?id=153132&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=153132&action=review


> Source/JavaScriptCore/offlineasm/parser.rb:94
> +	       annotationType = whitespaceFound ? "@L" : "@G" # local or global
annotation

It is customary to use symbols (:thing) rather than strings ("thing") for
enumerating in ruby.

> Source/JavaScriptCore/offlineasm/parser.rb:99
> -		   result << Token.new(CodeOrigin.new(fileName, lineNumber),
"@" + annotation)
> +		   result << Token.new(CodeOrigin.new(fileName, lineNumber),
> +				       "#{annotationType}#{annotation}")

I don't like the idea of using strings with magic as the contents of a token. 
Tokens are supposed to contain the string that was parsed.  We already have a
slightly ugly hack for numbers (we convert the string to a number then back to
a decimal string) but I don't want that to spread.