Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10894011:
Do not emit some unnecessary whitespaces in unparser. (Closed)
|
Created:
8 years, 3 months ago by Roman Modified:
8 years, 3 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionDo not emit some unnecessary whitespaces in unparser.
This reduces the size of dart2js minified bundle by 12K (417 -> 405), 3%
Committed: https://code.google.com/p/dart/source/detail?r=11611
Patch Set 1 #Patch Set 2 : #
Total comments: 11
Patch Set 3 : #
Total comments: 10
Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 12 (0 generated)
https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (left): https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:224: String delimiter = (node.delimiter === null) ? " " : "${node.delimiter}"; shouldn't we provide explicit empty string delimiter instead of fancy null conversion? https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:247: if (node.hasExpression && node.getBeginToken().slowToString() != '=>') { I don't like this difference between beginToken above and getBeginToken() here https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:344: if (node.body is !Block) sb.add(' '); shouldn't this logic belong to Block unparsing?
https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (left): https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:224: String delimiter = (node.delimiter === null) ? " " : "${node.delimiter}"; On 2012/08/30 10:24:16, Anton Muhin wrote: > shouldn't we provide explicit empty string delimiter instead of fancy null > conversion? I think it's just a matter of agreement. I didn't look at all possible NodeList-s. Maybe Peter can say something about it. https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:247: if (node.hasExpression && node.getBeginToken().slowToString() != '=>') { On 2012/08/30 10:24:16, Anton Muhin wrote: > I don't like this difference between beginToken above and getBeginToken() here Changed to beginToken.value https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:344: if (node.body is !Block) sb.add(' '); On 2012/08/30 10:24:16, Anton Muhin wrote: > shouldn't this logic belong to Block unparsing? body may be any statement, not necessary a block. Another approach to whitespaces in these cases would be adding some state flag like 'requires space' that token may set or reset or next visit method may take into account. But probably that would complicate things too much.
LGTM. Overall, this feels like it is cleaner than before, but it also feels like something more general would be nicer to work with. There are some cases you're not handling yet. Something like: if(b)x else(foo).bar() In this case, you'll add a space after else. But my guess is you couldn't measure that. https://chromiumcodereview.appspot.com/10894011/diff/4001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10894011/diff/4001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:172: if (node.elsePart is !Block) sb.add(' '); This could probably be generalized if we didn't use a string buffer, but used a custom class to collect "tokens". When these tokens are concatenated to produce the final string, you'd be able to easily see if a space is needed or not. https://chromiumcodereview.appspot.com/10894011/diff/4001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:247: if (node.hasExpression && node.getBeginToken().slowToString() != '=>') { Please do not use slowToString. That's why it has "slow" in its name. It should be: node.getBeginToken().stringValue !== '=>' Furthermore, getBeginToken() is not the right abstraction here. It is for computing the range of an AST node. This is why the previous line uses node.beginToken directly. A related remark: beginToken is a bad name. It should probably be called "returnKeyword", like LiteralList's "constKeyword". Anyways, I think it would be much nicer if the Return class had a bool getter named something like "isCompactReturn". https://chromiumcodereview.appspot.com/10894011/diff/4001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:250: visit(node.expression); Is this change intentional? https://chromiumcodereview.appspot.com/10894011/diff/4001/tests/compiler/dart... File tests/compiler/dart2js/unparser_test.dart (right): https://chromiumcodereview.appspot.com/10894011/diff/4001/tests/compiler/dart... tests/compiler/dart2js/unparser_test.dart:303: 'main(){p_globalVar;p_globalVarInitialized;p_globalVarInitialized2;p_globalfoo();' Long lines here. https://chromiumcodereview.appspot.com/10894011/diff/4001/tests/compiler/dart... tests/compiler/dart2js/unparser_test.dart:413: 'main(){var a=new A();a.foo();var b=new p_A.fromFoo();b.foo();var GREATVAR=b.myliba;b.mylist;a=getA();p_topfoo();topfoo();}'; Long line.
There are still cases when unnecessary whitespaces are added, but I just don't want to put any more hacks or complicate the code because for our purposes these remaining cases have negligible impact. https://chromiumcodereview.appspot.com/10894011/diff/4001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10894011/diff/4001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:247: if (node.hasExpression && node.getBeginToken().slowToString() != '=>') { On 2012/08/30 10:42:45, ahe wrote: > Please do not use slowToString. That's why it has "slow" in its name. > > It should be: > > node.getBeginToken().stringValue !== '=>' > > Furthermore, getBeginToken() is not the right abstraction here. It is for > computing the range of an AST node. This is why the previous line uses > node.beginToken directly. A related remark: beginToken is a bad name. It should > probably be called "returnKeyword", like LiteralList's "constKeyword". > > Anyways, I think it would be much nicer if the Return class had a bool getter > named something like "isCompactReturn". In the last patch I changed this to: node.beginToken.kind != FUNCTION_TOKEN https://chromiumcodereview.appspot.com/10894011/diff/4001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:250: visit(node.expression); On 2012/08/30 10:42:45, ahe wrote: > Is this change intentional? Yes. visit() checks for null anyway and I didn't want to place another if inside of existing if for adding whitespace.
https://chromiumcodereview.appspot.com/10894011/diff/4001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10894011/diff/4001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:247: if (node.hasExpression && node.getBeginToken().slowToString() != '=>') { On 2012/08/30 11:38:48, Roman wrote: > On 2012/08/30 10:42:45, ahe wrote: > > Please do not use slowToString. That's why it has "slow" in its name. > > > > It should be: > > > > node.getBeginToken().stringValue !== '=>' > > > > Furthermore, getBeginToken() is not the right abstraction here. It is for > > computing the range of an AST node. This is why the previous line uses > > node.beginToken directly. A related remark: beginToken is a bad name. It > should > > probably be called "returnKeyword", like LiteralList's "constKeyword". > > > > Anyways, I think it would be much nicer if the Return class had a bool getter > > named something like "isCompactReturn". > > In the last patch I changed this to: > node.beginToken.kind != FUNCTION_TOKEN Please don't use the _TOKEN variables. You really should use stringValue and string literals. https://chromiumcodereview.appspot.com/10894011/diff/4001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:250: visit(node.expression); On 2012/08/30 11:38:48, Roman wrote: > On 2012/08/30 10:42:45, ahe wrote: > > Is this change intentional? > > Yes. visit() checks for null anyway and I didn't want to place another if inside > of existing if for adding whitespace. Of course. I wasn't thinking.
https://chromiumcodereview.appspot.com/10894011/diff/4001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10894011/diff/4001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:247: if (node.hasExpression && node.getBeginToken().slowToString() != '=>') { On 2012/08/30 11:51:07, ahe wrote: > On 2012/08/30 11:38:48, Roman wrote: > > On 2012/08/30 10:42:45, ahe wrote: > > > Please do not use slowToString. That's why it has "slow" in its name. > > > > > > It should be: > > > > > > node.getBeginToken().stringValue !== '=>' > > > > > > Furthermore, getBeginToken() is not the right abstraction here. It is for > > > computing the range of an AST node. This is why the previous line uses > > > node.beginToken directly. A related remark: beginToken is a bad name. It > > should > > > probably be called "returnKeyword", like LiteralList's "constKeyword". > > > > > > Anyways, I think it would be much nicer if the Return class had a bool > getter > > > named something like "isCompactReturn". > > > > In the last patch I changed this to: > > node.beginToken.kind != FUNCTION_TOKEN > > Please don't use the _TOKEN variables. You really should use stringValue and > string literals. Changed to: node.beginToken.stringValue != '=>'
And about long lines in unparser_test - these strings change pretty often and nobody really reads them, it just takes much time to edit them to fit 80 chars without any real reward. If something goes wrong there I usually re-run the test with additional line breaks so I can actually read the output.
https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (left): https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:224: String delimiter = (node.delimiter === null) ? " " : "${node.delimiter}"; In this case, please, adjust interpolation case. On 2012/08/30 10:40:51, Roman wrote: > On 2012/08/30 10:24:16, Anton Muhin wrote: > > shouldn't we provide explicit empty string delimiter instead of fancy null > > conversion? > > I think it's just a matter of agreement. I didn't look at all possible > NodeList-s. Maybe Peter can say something about it. https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:344: if (node.body is !Block) sb.add(' '); Ok. On 2012/08/30 10:40:51, Roman wrote: > On 2012/08/30 10:24:16, Anton Muhin wrote: > > shouldn't this logic belong to Block unparsing? > > body may be any statement, not necessary a block. Another approach to > whitespaces in these cases would be adding some state flag like 'requires space' > that token may set or reset or next visit method may take into account. But > probably that would complicate things too much. https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:344: if (node.body is !Block) sb.add(' '); overall, instead of checking body type, shouldn't you check next token? If it's {, there is no need to emit a space, ditto for ( and alikes (which will solve Peter's concern nicely). Cannot we introduce a category of tokens which behave like whitespace after keywords and use this generic approach?
https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:344: if (node.body is !Block) sb.add(' '); On 2012/08/30 12:01:53, Anton Muhin wrote: > overall, instead of checking body type, shouldn't you check next token? If it's > {, there is no need to emit a space, ditto for ( and alikes (which will solve > Peter's concern nicely). > > Cannot we introduce a category of tokens which behave like whitespace after > keywords and use this generic approach? Well, ideally yes. But that would complicate things more than 2 if-s I introduced. I don't know if it's worth it.
https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10894011/diff/2001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:344: if (node.body is !Block) sb.add(' '); Up to you to decide. BTW, it won't complicate things a lot: you'll have same ifs as you have right now and a helper function. Anyway, as you wish On 2012/08/30 12:16:04, Roman wrote: > On 2012/08/30 12:01:53, Anton Muhin wrote: > > overall, instead of checking body type, shouldn't you check next token? If > it's > > {, there is no need to emit a space, ditto for ( and alikes (which will solve > > Peter's concern nicely). > > > > Cannot we introduce a category of tokens which behave like whitespace after > > keywords and use this generic approach? > > Well, ideally yes. But that would complicate things more than 2 if-s I > introduced. I don't know if it's worth it.
On 2012/08/30 11:57:38, Roman wrote: > And about long lines in unparser_test - these strings change pretty often and > nobody really reads them, it just takes much time to edit them to fit 80 chars > without any real reward. If something goes wrong there I usually re-run the test > with additional line breaks so I can actually read the output. Hi Roman, If have have to limit all the lines I write to 80 columns (which I think is ridiculous), then so do you :-) Cheers, Peter |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
