Issue 9107070: Proper handling of built-in identifiers

Issue 9107070: Proper handling of built-in identifiers (Closed)

Created:
8 years, 11 months ago by hausner

Modified:
8 years, 11 months ago

Reviewers:
siva

CC:
reviews_dartlang.org

Base URL:
http://dart.googlecode.com/svn/branches/bleeding_edge/dart/

Visibility:
Public.

More Reviews

Description

Proper handling of built-in identifiers Allow pseudo-keywords (built-in identifiers) as regular identifiers for anything but class names, interface names, typedef names. Still todo: Update blacklist of class/interface names, disallow pseudo-keywords as type parameter names. Committed: https://code.google.com/p/dart/source/detail?r=3303

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 8

Patch Set 3 : '' #

Created: 8 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+156 lines, -108 lines)			Patch
M	runtime/vm/parser.h	View	1 2	4 chunks	+4 lines, -1 line	0 comments	Download
M	runtime/vm/parser.cc	View	1 2	47 chunks	+93 lines, -77 lines	0 comments	Download
M	runtime/vm/token.h	View	1 2	4 chunks	+7 lines, -3 lines	0 comments	Download
M	tests/language/language.status	View	1 2	1 chunk	+6 lines, -9 lines	0 comments	Download
M	tests/language/src/NamingTest.dart	View	1 2	3 chunks	+1 line, -18 lines	0 comments	Download
M	tests/language/src/PseudoKWTest.dart	View	1 2	1 chunk	+45 lines, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

siva

LGTM http://codereview.chromium.org/9107070/diff/4001/runtime/vm/parser.cc File runtime/vm/parser.cc (right): http://codereview.chromium.org/9107070/diff/4001/runtime/vm/parser.cc#newcode5343 runtime/vm/parser.cc:5343: !Token::IsPseudoKeyword(CurrentToken())) { Why not if (!IsIdentifier()) { ErrorMsg(msg); ...

8 years, 11 months ago (2012-01-13 18:48:19 UTC) #2

hausner

8 years, 11 months ago (2012-01-13 19:22:47 UTC) #3

Thank you

http://codereview.chromium.org/9107070/diff/4001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

http://codereview.chromium.org/9107070/diff/4001/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:5343: !Token::IsPseudoKeyword(CurrentToken())) {
On 2012/01/13 18:48:20, asiva wrote:
> Why not
> if (!IsIdentifier()) {
>   ErrorMsg(msg);
> }
Excellent idea. I introduced IsIdentifier after I changed this code, and it
didn't appear to me.

http://codereview.chromium.org/9107070/diff/4001/tests/language/src/NamingTes...
File tests/language/src/NamingTest.dart (right):

http://codereview.chromium.org/9107070/diff/4001/tests/language/src/NamingTes...
tests/language/src/NamingTest.dart:183: }
On 2012/01/13 18:48:20, asiva wrote:
> I guess you removed this because it is a duplicate definition, Shouldn't we
move
> it into a negative test?
Yes, the test was just wrong.

http://codereview.chromium.org/9107070/diff/4001/tests/language/src/NamingTes...
tests/language/src/NamingTest.dart:514: }
On 2012/01/13 18:48:20, asiva wrote:
> Ditto comment.

Ditto answer.

http://codereview.chromium.org/9107070/diff/4001/tests/language/src/PseudoKWT...
File tests/language/src/PseudoKWTest.dart (right):

http://codereview.chromium.org/9107070/diff/4001/tests/language/src/PseudoKWT...
tests/language/src/PseudoKWTest.dart:77: set get(x) => abstract = 2 * x;
On 2012/01/13 18:48:20, asiva wrote:
> set get(B get) => abstract = 2 * get.get?

Nice :) I made it even more pathological and promptly found an case that wasn't
handled correctly (fields definitions of name get, set or operator).

Expand Messages | Collapse Messages