Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
469213009:
Fix in compiler pass: cr.define() can export variable without assigned value (Closed)
|
Created:
6 years, 4 months ago by Vitaly Pavlenko Modified:
6 years, 4 months ago CC:
chromium-reviews, dbeam+watch-closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@true_master Project:
chromium Visibility:
Public. |
DescriptionFix in compiler pass: cr.define() can export variable without assigned value
BUG=393873
R=tbreisacher@chromium.org,dbeam@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290712
Patch Set 1 #
Total comments: 6
Patch Set 2 : add test to show behavior on unexported variables #
Messages
Total messages: 21 (0 generated)
This is weird. Do you have an example of where code is actually doing this? It's not clear why you'd want to do it. https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... File third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java (right): https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:107: "});\n", You're already using Guava elsewhere so you should be able to use a Joiner here, I think. test(Joiner.on('\n').join( "first line of code", "second line", "etc."), Joiner.on('\n').join( "var namespace = ...", "etc.")); A lot nicer to read than all the \n's and +'s :)
On 2014/08/18 16:51:00, Tyler Breisacher (Chromium) wrote: > This is weird. Do you have an example of where code is actually doing this? It's > not clear why you'd want to do it. > An example of such code is here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... I don't know if there's any code like that in other files. In this particular case we can just remove this declaration in JS source. Dan, what do you think?
Interesting. The code does use 'bmm.tree' at line 166, but the local 'tree' var is not used. I suppose you want to leave the JS as is, otherwise you'd get an error at that line that the bmm namespace has no 'tree' property. So I guess leave it as is for now?
lgtm
https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... File third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java (right): https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:121: " var a;" + so is this only change from a => namespace.a in the case that it's exported?
https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... File third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java (right): https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:121: " var a;" + On 2014/08/18 18:36:46, Dan Beam wrote: > so is this only change from a => namespace.a in the case that it's exported? said in another way cr.define('ui', function() { var a = 1; var b = 2; return {b: b}; }); |ui.a| will be undefined and |ui.b| == 2, right?
On 2014/08/18 18:57:45, Dan Beam wrote: > https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... > File > third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java > (right): > > https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... > third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:121: > " var a;" + > On 2014/08/18 18:36:46, Dan Beam wrote: > > so is this only change from a => namespace.a in the case that it's exported? > > said in another way > > cr.define('ui', function() { > var a = 1; > var b = 2; > return {b: b}; > }); > > |ui.a| will be undefined and |ui.b| == 2, right? Yes, |ui.a| won't be visible externally to the compiler. So that's all about definition.
On 2014/08/18 19:03:21, Vitaly Pavlenko wrote: > On 2014/08/18 18:57:45, Dan Beam wrote: > > > https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... > > File > > > third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java > > (right): > > > > > https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... > > > third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:121: > > " var a;" + > > On 2014/08/18 18:36:46, Dan Beam wrote: > > > so is this only change from a => namespace.a in the case that it's exported? > > > > said in another way > > > > cr.define('ui', function() { > > var a = 1; > > var b = 2; > > return {b: b}; > > }); > > > > |ui.a| will be undefined and |ui.b| == 2, right? > > Yes, |ui.a| won't be visible externally to the compiler. So that's all about > definition. great, can you update the tests to this effect? add a local var and ensure it doesn't get re-written.
On 2014/08/18 19:05:58, Dan Beam wrote: > On 2014/08/18 19:03:21, Vitaly Pavlenko wrote: > > On 2014/08/18 18:57:45, Dan Beam wrote: > > > > > > https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... > > > File > > > > > > third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java > > > (right): > > > > > > > > > https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... > > > > > > third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:121: > > > " var a;" + > > > On 2014/08/18 18:36:46, Dan Beam wrote: > > > > so is this only change from a => namespace.a in the case that it's > exported? > > > > > > said in another way > > > > > > cr.define('ui', function() { > > > var a = 1; > > > var b = 2; > > > return {b: b}; > > > }); > > > > > > |ui.a| will be undefined and |ui.b| == 2, right? > > > > Yes, |ui.a| won't be visible externally to the compiler. So that's all about > > definition. > > great, can you update the tests to this effect? add a local var and ensure it > doesn't get re-written. Done.
lgtm https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... File third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java (right): https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:107: "});\n", On 2014/08/18 16:51:00, Tyler Breisacher (Chromium) wrote: > You're already using Guava elsewhere where are we using Guava? could we avoid adding new dependencies if we use compiler.jar to the Class-path? > so you should be able to use a Joiner here, > I think. > > test(Joiner.on('\n').join( > "first line of code", > "second line", > "etc."), Joiner.on('\n').join( > "var namespace = ...", > "etc.")); > > A lot nicer to read than all the \n's and +'s :) eh
https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... File third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java (right): https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:107: "});\n", On 2014/08/19 21:14:19, Dan Beam wrote: > where are we using Guava? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... I'm not sure how the dependencies work in Chrome, but there is also a copy of Guava at https://code.google.com/p/chromium/codesearch#chromium/src/third_party/guava/
https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... File third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java (right): https://chromiumcodereview.appspot.com/469213009/diff/1/third_party/closure_c... third_party/closure_compiler/runner/test/com/google/javascript/jscomp/ChromePassTest.java:107: "});\n", On 2014/08/19 21:19:47, Tyler Breisacher (Chromium) wrote: > On 2014/08/19 21:14:19, Dan Beam wrote: > > where are we using Guava? > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... > > I'm not sure how the dependencies work in Chrome, but there is also a copy of > Guava at > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/guava/ i assume that works because of: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur...
The CQ bit was checked by vitalyp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalyp@chromium.org/469213009/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by vitalyp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalyp@chromium.org/469213009/20001
Message was sent while issue was closed.
Committed patchset #2 (20001) as 290712 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
