Thanks, Lex! On Tue, Apr 7, 2009 at 8:01 AM, Lex Spoon <sp...@google.com> wrote:
> This is a big improvement on the logging. I really like the > gist of it. I think it should have a second iteration, though. > > I reluctantly agree about dropping most all warnings. Once we have a > way to suppress warnings, then we can talk about how to put them back > in. > The big thing here was the "expected" warnings, for example from the nonserializable subtypes of list. I wanted to differentiate user from non-user code (which may be a suppression regexp, for example), but deferred that for now. > The main thing is that many problems are still logged via TreeLogger > and not stored in the ProblemReport. Shouldn't we jump over > consistently to the new system rather than have a mix? Are there any Probably; I was worried that I might lose something valuable (for example, if we put a log message in ProblemReport and then drop it as above). That shouldn't be an issue if we're selective about which TreeLogger logs migrate; I just wanted to get the focal thing working first. Happy to do a second revision. > Also, the new "visited" field is redundant with the typeToTyeInfoComputed > map. > It looks like you ran into some recursion issue somewhere. We should > fix that problem by improving the existing short-circuiting via TICs, not > by adding a new and subtly different field. > Yeah, I suspected this would come up. I'll take a closer look at using TIC instead. There's no special recursion I had to provoke; if you put logging in instead of my short circuit, I think DynaTable reconsiders java.lang.String something like 23 times before reaching TIC shortcircuits. My patch saved almost 10% time for DynaTable, I think due to this earlier shortcircuit. (Sorry, should have bragged on that in the first posting.) > Error message content: > > I have not looked at the output carefully, but it looks like we are > losing dependency information between types. This could be improved > greatly by adding link-up messages to any type that fails due to a > separate type from failing. In particular: > > 1. In checkSubtypes(), add a problem for each candidate that is > rejected. For example, "tried subtype Foo but it was not > instantiable". > This one I think we don't lose, because of the context in the ProblemReport. We get messages like "com.foocorp.SubFoo had no default instructor (reached via com.foocorp.Foo)". The complete chain of subtypes, yes, you'd need debug logging to see. > 2. In checkSubtype(), log messages for recursing to a supertype and > recursing into a field's type. For example, "superclass Foo is not > instantiable". "Field x of type Foo is not instantiable". > You similarly get the context type from your actual interface here, but yes, in these cases it might be more confusing how you got from here to there. > It would greatly help if the report sorted the types by their fully > qualified name. > Ok. Nits: > > To simplify the code and help with future developments, the errors > could be stored in the TIC class instead of in ProblemReport. Simply > add a field "List<String> problems". The two static methods that take > a ProblemReport as an argument could instead take a List as an > argument. The general rule is that TIC holds the information STOB > deduces about a particular type. Surely it will work well to log > the errors there directly. > The reason I was hesitant here was memory pressure... I wasn't sure how long-lived the STOB and TIC info was (yes, I should have worked it out). The purpose to ProblemReport is to become garbage relatively quickly. > You should delete > it, revive it, and delete it again, and then upload the video to > YouTube. I never taunt dead code, for fear it will come back to haunt me... --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
