Opened 13 years ago
Closed 13 years ago
#8828 closed enhancement (fixed)
[ccla][patch]Shrinksafe could munge strings together
Reported by: | dante | Owned by: | dante |
---|---|---|---|
Priority: | high | Milestone: | 1.4 |
Component: | ShrinkSafe | Version: | 1.3.0b2 |
Keywords: | Cc: | Adam Peller, Richard Backhouse | |
Blocked By: | Blocking: |
Description
It has been suggested in the forums that shrinksafe be enhanced to handle removing useless string concatenation as a result of newlines:
var a = "b" + "c" + "d";
should just be var a = "bcd";
but should not fail on: var a = "b" + c + "d" + "e";
should be: var a = "b"+c+"de";
patch and unit tests attached.
Attachments (5)
Change History (17)
Changed 13 years ago by
Attachment: | mungeStrings.patch added |
---|
Changed 13 years ago by
Attachment: | mungeStrings.2.patch added |
---|
comment:1 Changed 13 years ago by
would it be possible/preferable to do this with tokens rather than a full pass with a regexp?
comment:2 Changed 13 years ago by
Component: | General → ShrinkSafe |
---|
Changed 13 years ago by
Attachment: | concat.patch added |
---|
concat strings in compress method from tokens
comment:4 Changed 13 years ago by
Summary: | Shrinksafe could munge strings together → [ccla][patch]Shrinksafe could munge strings together |
---|
comment:5 Changed 13 years ago by
Status: | new → assigned |
---|
@peller - thanks for this patch, it's great! seems to work perfectly. I've got most of a new unit test going, trying to think of more cases to check. Will likely commit patch and tests this evening.
comment:6 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
let a sleeping trac, sleep. fixed in [19078]
comment:7 Changed 13 years ago by
hmm... what I did with the tokenizer is right 99% of the time, but not strictly valid.
!"a"+"b"
should yield
"falseb"
not
false
or !"ab" Perhaps we really need an AST to perform these types of optimizations?
Changed 13 years ago by
Attachment: | 8828regression.patch added |
---|
A regression against my own patch. Granted, it looks like an edge case, but Shrinksafe ought to be safe (this could end up being a frustrating typo that shrinksafe would mask)
comment:8 Changed 13 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:9 Changed 13 years ago by
I tested:
(function(){ var x = !"a" + "b"; })();
with YUICompressor, and the output was:
(function(){var a=!"ab";})();
so this "bug"/limitation exists there (not justifying the fail, just interesting that it exists there, too.
the overall savings of this patch against dijit-all.js (only templateString inline / pre-build) uses multi-line concatenation like this) is minimal: 60 bytes or so.
I would, however, like to offer this option as a cmd line toggle perhaps? classify this string munging as a "microoptimiztion that could potentially break code". We similarly break on "complex console.* stripping", though neek has a patch attempting to fix that.
comment:10 follow-up: 11 Changed 13 years ago by
dante: you mention this saves 60 bytes on dijit-all.js, what is the total file size with the 60 bytes? This file: http://o.aolcdn.com/dojo/1.3/dijit/dijit-all.js
is 83KB gzipped. So a 60 byte savings is like 0.07% file savings. I would say it is not worth providing the option at all, given the extra possibility of error, the extra explaining of the option, and a new feature to support.
comment:11 Changed 13 years ago by
Replying to jburke:
dante: you mention this saves 60 bytes on dijit-all.js, what is the total file size with the 60 bytes?
To me this optimization is about run-time cost savings: no expensive concatenations. But if it doesn't work in some cases, I think we should skip it to save our users from frustrating debugging efforts. I am saying it as someone who debugged build problems once or twice. :-(
comment:12 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
updated patch adds new unit test