Issue 1867243004: Begin refactoring compiler out of diet parser and scanner

Issue 1867243004: Begin refactoring compiler out of diet parser and scanner (Closed)

Created:
4 years, 8 months ago by Harry Terkelsen

Modified:
4 years, 8 months ago

Reviewers:
Siggi Cherem (dart-lang)

CC:
reviews_dartlang.org

Base URL:
git@github.com:dart-lang/sdk.git@master

Target Ref:
refs/heads/master

Visibility:
Public.

More Reviews

Description

Begin refactoring compiler out of diet parser and scanner BUG= R=sigmund@google.com Committed: https://github.com/dart-lang/sdk/commit/9f8427d041b99a3749fcaf461726e01db1fc17a8

Patch Set 3 : #

Created: 4 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+86 lines, -52 lines)			Patch
M	pkg/compiler/lib/src/closure.dart	View		1 chunk	+1 line, -1 line	0 comments	Download
M	pkg/compiler/lib/src/compiler.dart	View	1 2	7 chunks	+9 lines, -28 lines	0 comments	Download
A	pkg/compiler/lib/src/id_generator.dart	View	1 2	1 chunk	+13 lines, -0 lines	0 comments	Download
M	pkg/compiler/lib/src/parser/diet_parser_task.dart	View		2 chunks	+5 lines, -3 lines	0 comments	Download
M	pkg/compiler/lib/src/parser/element_listener.dart	View		5 chunks	+4 lines, -5 lines	0 comments	Download
M	pkg/compiler/lib/src/patch_parser.dart	View		3 chunks	+4 lines, -4 lines	0 comments	Download
M	pkg/compiler/lib/src/resolution/class_hierarchy.dart	View		1 chunk	+1 line, -1 line	0 comments	Download
M	pkg/compiler/lib/src/scanner/scanner_task.dart	View	1 2	3 chunks	+45 lines, -7 lines	0 comments	Download
M	tests/compiler/dart2js/exit_code_test.dart	View	1 2	1 chunk	+2 lines, -1 line	0 comments	Download
M	tests/compiler/dart2js/parser_helper.dart	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download

Messages

Total messages: 8 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Siggi Cherem (dart-lang)

lgtm! really nice! https://codereview.chromium.org/1867243004/diff/20001/pkg/compiler/lib/src/compiler.dart File pkg/compiler/lib/src/compiler.dart (right): https://codereview.chromium.org/1867243004/diff/20001/pkg/compiler/lib/src/compiler.dart#newcode92 pkg/compiler/lib/src/compiler.dart:92: abstract class Compiler implements LibraryLoaderListener, IdGenerator ...

4 years, 8 months ago (2016-04-08 19:37:56 UTC) #3

Harry Terkelsen

https://codereview.chromium.org/1867243004/diff/20001/pkg/compiler/lib/src/compiler.dart File pkg/compiler/lib/src/compiler.dart (right): https://codereview.chromium.org/1867243004/diff/20001/pkg/compiler/lib/src/compiler.dart#newcode92 pkg/compiler/lib/src/compiler.dart:92: abstract class Compiler implements LibraryLoaderListener, IdGenerator { On 2016/04/08 ...

4 years, 8 months ago (2016-04-08 20:52:42 UTC) #4

https://codereview.chromium.org/1867243004/diff/20001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/1867243004/diff/20001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/compiler.dart:92: abstract class Compiler implements
LibraryLoaderListener, IdGenerator {
On 2016/04/08 19:37:56, Siggi Cherem (dart-lang) wrote:
> what are your thoughts on splitting it up in two objects:
> 
> class Compiler {
>   IdGenerator idGenerator = new IdGenerator();
> 
>   ...
> }
> 
> ?
> 
> Technically the real goal is to split it up where it is used (e.g.
> ClosureClassElement should take an idGenerator eventually), so it's not
> necessary to split it here too. But I wonder if it will make it easier in the
> future... 

Done. But I had to keep Compiler as an implementor of IdGenerator because some
places (like ClosureClassElement) call getNextId directly from compiler

https://codereview.chromium.org/1867243004/diff/20001/pkg/compiler/lib/src/sc...
File pkg/compiler/lib/src/scanner/scanner_task.dart (right):

https://codereview.chromium.org/1867243004/diff/20001/pkg/compiler/lib/src/sc...
pkg/compiler/lib/src/scanner/scanner_task.dart:24: ScannerTask(Compiler
compiler, this._dietParser, this._commentMap,
On 2016/04/08 19:37:56, Siggi Cherem (dart-lang) wrote:
> consider making _commentMap an optional argument too? (it's only needed when
you
> preserveComments)

Done.

https://codereview.chromium.org/1867243004/diff/20001/pkg/compiler/lib/src/sc...
pkg/compiler/lib/src/scanner/scanner_task.dart:26: : _preserveComments =
preserveComments,
On 2016/04/08 19:37:56, Siggi Cherem (dart-lang) wrote:
> ... I wish the language just did this public/private name translation
> automatically from this._preserveComments.

+1! This an annoying and common problem

https://codereview.chromium.org/1867243004/diff/20001/pkg/compiler/lib/src/sc...
pkg/compiler/lib/src/scanner/scanner_task.dart:73: Token
processAndStripComments(Token currentToken) {
On 2016/04/08 19:37:56, Siggi Cherem (dart-lang) wrote:
> I really like what you did here to contain comments within the scanner.
> FYI - today this is only used for source_mirrors, but we should definitely
keep
> it around for the future. 

Acknowledged.

Harry Terkelsen

Thanks, Siggi. PTAL because I changed a couple of things around

4 years, 8 months ago (2016-04-08 20:53:12 UTC) #5

Harry Terkelsen

4 years, 8 months ago (2016-04-08 21:11:24 UTC) #8

Message was sent while issue was closed.

Committed patchset #3 (id:40001) manually as
9f8427d041b99a3749fcaf461726e01db1fc17a8 (presubmit successful).

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages