[gwt-contrib] Re: Instant Hosted Mode
Awesome, thanks.
http://gwt-code-reviews.appspot.com/51826/diff/1/65
File dev/core/src/com/google/gwt/dev/javac/JSORestrictionsChecker.java
(right):
http://gwt-code-reviews.appspot.com/51826/diff/1/65#newcode203
Line 203: private void fail() {
I just meant this visitor itself could throw the abort and catch it in
the calling code. That would still let you report multiple non-empty
ctors.
http://gwt-code-reviews.appspot.com/51826/diff/1/65#newcode322
Line 322: * If it is a non-static inner class (which we will give an
error for
On 2009/07/31 01:36:23, jat wrote:
> If we abort after the first error, we may make the user make multiple
passes
> before getting it right, rather than catching all the errors up front.
I always
> hated compilers that worked that way, but if you think the savings is
> significant enough to justify that behavior I can do it that way.
The thing is, anyone who tries to make a non-static inner class a JSO is
definitely on the wrong track. Since we'll be pointing them at the
error doc that gives all the rules, I think it's ok to have the user
two-pass this one case to simplify our code.
> Moving the state transitions to the enum also means moving the logic
that goes
> with those transitions (and the values that logic operates on) to the
enum,
> which didn't seem appropriate to me.
Was just a thought.
http://gwt-code-reviews.appspot.com/51826/diff/1/65#newcode330
Line 330: protected enum CtorState {
> Can't have an enum inside a non-static class, though if we make the
visitor
> static instead that could be done.
STGM. The visitor can be static if you throw the abort out.
http://gwt-code-reviews.appspot.com/51826/diff/1/71
File dev/core/src/com/google/gwt/dev/shell/CompilingClassLoader.java
(right):
http://gwt-code-reviews.appspot.com/51826/diff/1/71#newcode1098
Line 1098: assert unit != null;
Yeah, in fact, feel free to commit this directly to trunk.
http://gwt-code-reviews.appspot.com/51826
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Instant Hosted Mode
Thanks for the review.
http://gwt-code-reviews.appspot.com/51826/diff/1/77
File dev/core/src/com/google/gwt/dev/SwtHostedModeBase.java (right):
http://gwt-code-reviews.appspot.com/51826/diff/1/77#newcode72
Line 72: TypeOracle typeOracle = moduleDef.getTypeOracle(logger,
wantBinaries);
Ok.
http://gwt-code-reviews.appspot.com/51826/diff/1/76
File dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java (right):
http://gwt-code-reviews.appspot.com/51826/diff/1/76#newcode58
Line 58: protected final String __binary_1_path = "";
I will get back to you on that -- when I first added this, I had a use
case in mind that required being able to specify it separately from
source, but I can't remember it right now.
http://gwt-code-reviews.appspot.com/51826/diff/1/65
File dev/core/src/com/google/gwt/dev/javac/JSORestrictionsChecker.java
(left):
http://gwt-code-reviews.appspot.com/51826/diff/1/65#oldcode344
Line 344: GWTProblem.recordInCud(node, cud, error, new
InstalledHelpInfo(
Yes -- I will add it back.
http://gwt-code-reviews.appspot.com/51826/diff/1/65
File dev/core/src/com/google/gwt/dev/javac/JSORestrictionsChecker.java
(right):
http://gwt-code-reviews.appspot.com/51826/diff/1/65#newcode203
Line 203: private void fail() {
You would only get one message even if there are multiple non-empty
constructors.
For it to be useful to get multiple errors, I was thinking of adding
more explicit descriptions of the constructors that had an error, but if
we don't mind only getting one error where the code has multiple errors
we could include that in the exception.
http://gwt-code-reviews.appspot.com/51826/diff/1/65#newcode322
Line 322: * If it is a non-static inner class (which we will give an
error for
If we abort after the first error, we may make the user make multiple
passes before getting it right, rather than catching all the errors up
front. I always hated compilers that worked that way, but if you think
the savings is significant enough to justify that behavior I can do it
that way.
Moving the state transitions to the enum also means moving the logic
that goes with those transitions (and the values that logic operates on)
to the enum, which didn't seem appropriate to me.
http://gwt-code-reviews.appspot.com/51826/diff/1/65#newcode330
Line 330: protected enum CtorState {
On 2009/07/31 01:00:33, scottb wrote:
> Can we make this a static inner class of EmptyConstructorChecker?
Can't have an enum inside a non-static class, though if we make the
visitor static instead that could be done.
http://gwt-code-reviews.appspot.com/51826/diff/1/71
File dev/core/src/com/google/gwt/dev/shell/CompilingClassLoader.java
(right):
http://gwt-code-reviews.appspot.com/51826/diff/1/71#newcode1098
Line 1098: assert unit != null;
Well, it isn't directly related to IHM, but it is a bug in that
getJsniMethods can return an empty list but we still go through the
effort of injecting them. At some point the code was changed possibly
return an empty list and this code wasn't updated to match.
The assert seemed useful (at least for me while I was debugging this),
but I can move this to a separate patch.
http://gwt-code-reviews.appspot.com/51826/diff/1/69
File dev/core/src/com/google/gwt/dev/shell/jetty/JettyLauncher.java
(right):
http://gwt-code-reviews.appspot.com/51826/diff/1/69#newcode1
Line 1: /*
Correct, I will separate it out.
http://gwt-code-reviews.appspot.com/51826/diff/1/80
File dev/core/src/com/google/gwt/dev/util/Name.java (right):
http://gwt-code-reviews.appspot.com/51826/diff/1/80#newcode1
Line 1: /*
Ok. There are also a bunch of other places where this should be used,
though that will probably be left for later.
http://gwt-code-reviews.appspot.com/51826/diff/1/80#newcode213
Line 213: public static String lookupBinaryName(String sourceName,
I will remove it.
http://gwt-code-reviews.appspot.com/51826/diff/1/78
File dev/core/src/com/google/gwt/dev/util/UnitTestTreeLogger.java
(right):
http://gwt-code-reviews.appspot.com/51826/diff/1/78#newcode186
Line 186: if (expectedEntries.size() != actualEntries.size()) {
Correct, other than I was getting failures here and the existing error
message wasn't helpful to understanding what went wrong.
I will move it to a separate patch.
http://gwt-code-reviews.appspot.com/51826/diff/1/3
File dev/linux/src/com/google/gwt/dev/shell/moz/ModuleSpaceMoz.java
(right):
http://gwt-code-reviews.appspot.com/51826/diff/1/3#newcode128
Line 128: * @param file the file name containing the JSNI
Ok, separate patch (perhaps we have different Eclipse settings, because
having a bogus @param generated an error for me).
http://gwt-code-reviews.appspot.com/51826/diff/1/82
File user/src/com/google/gwt/junit/JUnitShell.java (right):
http://gwt-code-reviews.appspot.com/51826/diff/1/82#newcode423
Line 423: // TODO(jat): how to make JUnitShell use binaries where
appropriate?
Yes, if we just automatically use binaries and use the system property
globally there is nothing to do here.
http://gwt-code-rev
[gwt-contrib] Re: Instant Hosted Mode
John, here's what I have so far, but I gotta run. I didn't
fine-tooth-comb JSORestrictionsChecker, but I'm sure it's good enough to
go it.
Some of my other concerns I would like to see resolved. With a commit
this big I think it makes sense to break it up to the extent possible.
I also did not really look too much at the stuff in dev.javac,
dev.resource, and TypeOracle, since I know most of that is big rewrites.
I was largely looking at the impact on the rest of the system.
http://gwt-code-reviews.appspot.com/51826/diff/1/2
File branch-info.txt (right):
http://gwt-code-reviews.appspot.com/51826/diff/1/2#newcode1
Line 1: Created from tr...@r5326
Please remove before committing. :)
http://gwt-code-reviews.appspot.com/51826/diff/1/77
File dev/core/src/com/google/gwt/dev/SwtHostedModeBase.java (right):
http://gwt-code-reviews.appspot.com/51826/diff/1/77#newcode72
Line 72: TypeOracle typeOracle = moduleDef.getTypeOracle(logger,
wantBinaries);
I basically disagree with threading this through all the apis. If you
want to leave an escape hatch in CompilationState itself, that's fine.
I would recommend we follow the existing conventions and set a private
static final boolean constant to
System.getProperty("gwt.typeOracle.noUseClasses") != null
http://gwt-code-reviews.appspot.com/51826/diff/1/76
File dev/core/src/com/google/gwt/dev/cfg/ModuleDefSchema.java (right):
http://gwt-code-reviews.appspot.com/51826/diff/1/76#newcode58
Line 58: protected final String __binary_1_path = "";
Why are we going to all this trouble to make it configurable? Shouldn't
it just always match the source packages?
http://gwt-code-reviews.appspot.com/51826/diff/1/76#newcode612
Line 612: logger.log(TreeLogger.WARN, "Non-relative public package: "
'public' is wrong
+2 more instances
http://gwt-code-reviews.appspot.com/51826/diff/1/76#newcode834
Line 834: private final class IncludeExcludeSchema extends Schema {
Don't do that.
+2 more occurrences in this file.
http://gwt-code-reviews.appspot.com/51826/diff/1/76#newcode1248
Line 1248: bodySchema.addBinaryPackage(modulePackageAsPath, "",
Empty.STRINGS,
Why wouldn't the default be 'client' to match the source path?
http://gwt-code-reviews.appspot.com/51826/diff/1/65
File dev/core/src/com/google/gwt/dev/javac/JSORestrictionsChecker.java
(left):
http://gwt-code-reviews.appspot.com/51826/diff/1/65#oldcode344
Line 344: GWTProblem.recordInCud(node, cud, error, new
InstalledHelpInfo(
Did you nuke this checker's ability to produce a link to the help doc?
http://gwt-code-reviews.appspot.com/51826/diff/1/65
File dev/core/src/com/google/gwt/dev/javac/JSORestrictionsChecker.java
(right):
http://gwt-code-reviews.appspot.com/51826/diff/1/65#newcode203
Line 203: private void fail() {
Just for this visitor, it seems to make the most sense for this class to
be static and just throw an abort out the top if you fail, catch and add
the error there.
http://gwt-code-reviews.appspot.com/51826/diff/1/65#newcode322
Line 322: * If it is a non-static inner class (which we will give an
error for
If we hit a non-static inner class, let's just abort and stop
processing. Wouldn't that make EmptyConstructorChecker simpler?
Also, the implementation might be somewhat easier to follow if somehow
we could encode the correct state transitions into the enum members.
But maybe that's farfetched.
http://gwt-code-reviews.appspot.com/51826/diff/1/65#newcode330
Line 330: protected enum CtorState {
Can we make this a static inner class of EmptyConstructorChecker?
http://gwt-code-reviews.appspot.com/51826/diff/1/71
File dev/core/src/com/google/gwt/dev/shell/CompilingClassLoader.java
(right):
http://gwt-code-reviews.appspot.com/51826/diff/1/71#newcode1098
Line 1098: assert unit != null;
Unrelated change?
http://gwt-code-reviews.appspot.com/51826/diff/1/70
File dev/core/src/com/google/gwt/dev/shell/ShellModuleSpaceHost.java
(right):
http://gwt-code-reviews.appspot.com/51826/diff/1/70#newcode1
Line 1: /*
This file gets a straight revert if we unthread the system property to
disable class reads.
http://gwt-code-reviews.appspot.com/51826/diff/1/69
File dev/core/src/com/google/gwt/dev/shell/jetty/JettyLauncher.java
(right):
http://gwt-code-reviews.appspot.com/51826/diff/1/69#newcode1
Line 1: /*
I'm sure this is a great patch, but it doesn't belong with IHM, right?
http://gwt-code-reviews.appspot.com/51826/diff/1/80
File dev/core/src/com/google/gwt/dev/util/Name.java (right):
http://gwt-code-reviews.appspot.com/51826/diff/1/80#newcode1
Line 1: /*
What would be kind of awesome is to get this class, plus the changes to
callers of this class that aren't related to IHM (CCL, for example) in
first as an initial commit.
There are actually several files with no IHM-related changes at all,
they're only being modded for the sake of using this class.
http://gwt-code-reviews.appspot.com/51826/diff/1/80#newcode213
Line 213: public static String lookupBinaryName(String sourceName,
This method really doesn't belong in this
