Typically, you should name a reviewer up front. I didn't see one in the initial mail, though I see Lex's name now, so maybe that got added later, or Lex jumped on it too.
http://gwt-code-reviews.appspot.com/47802/diff/1/6 File dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java (right): http://gwt-code-reviews.appspot.com/47802/diff/1/6#newcode30 Line 30: private boolean enableAssertions = true; Doesn't this turn assertions on by default ALL THE TIME, i.e. also for web mode production (where I think we don't want it)? I was expecting to just see the -ea flag used in common.ant.xml's gwt.junit macro, or twiddling in JUnitShell to set an enabled default there, but not globally. http://gwt-code-reviews.appspot.com/47802/diff/1/7 File dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerDisableAssertions.java (right): http://gwt-code-reviews.appspot.com/47802/diff/1/7#newcode30 Line 30: return "Debugging: disables the compiled output to check assert statements."; non-grammatical. How's "disables checking ... in the compiled output" or "causes the compiled output not to check..." http://gwt-code-reviews.appspot.com/47802/diff/1/2 File dev/core/test/com/google/gwt/dev/CompilerTest.java (right): http://gwt-code-reviews.appspot.com/47802/diff/1/2#newcode71 Line 71: assertTrue(options.isEnableAssertions()); Again, I'm not sure we meant to change the global default, rather than just for tests. http://gwt-code-reviews.appspot.com/47802/diff/1/5 File dev/core/test/com/google/gwt/dev/GWTCompilerTest.java (right): http://gwt-code-reviews.appspot.com/47802/diff/1/5#newcode65 Line 65: assertTrue(options.isEnableAssertions()); ditto to others. http://gwt-code-reviews.appspot.com/47802/diff/1/3 File dev/core/test/com/google/gwt/dev/GWTShellTest.java (right): http://gwt-code-reviews.appspot.com/47802/diff/1/3#newcode88 Line 88: assertTrue(options.isEnableAssertions()); ditto. http://gwt-code-reviews.appspot.com/47802/diff/1/4 File dev/core/test/com/google/gwt/dev/HostedModeTest.java (right): http://gwt-code-reviews.appspot.com/47802/diff/1/4#newcode134 Line 134: assertTrue(options.isEnableAssertions()); ditto. http://gwt-code-reviews.appspot.com/47802/diff/1/8 File user/src/com/google/gwt/junit/JUnitShell.java (right): http://gwt-code-reviews.appspot.com/47802/diff/1/8#newcode100 Line 100: Somewhere around here I think you want to do something to flip the "normal" default from -ea from false to true. Maybe you make this ArgProcessor have an implicit -ea before any args the user sets, so an explicit -da is needed to reverse that. Or maybe JUnitShell sets is own options.setEnableAssertions(true) even before calling the processor, which is probably cleaner. But the point is, the "true" default should be specific to JUnitShell, not the other program entry points. http://gwt-code-reviews.appspot.com/47802/diff/1/8#newcode117 Line 117: registerHandler(new ArgHandlerDisableAssertions(options)); If we do change the global default, this MUST be a compiler arg. How else could I turn assertions off in my web deploy? Even if we don't change the global, and so this COULD be a JUnitShell arg, I think we might still WANT it to be a compiler arg, to keep -da and -ea symmetrical. http://gwt-code-reviews.appspot.com/47802 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
