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 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 particular cases you ran into where it was hard to assign the problem to a particular type? In particular, the four callers to getLogLevel() look like they should be updated. 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. 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". 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". It could be that I misunderstand what the output will look like, though. If you think these messages would be overkill, could you post some error traces for cases like these? It would greatly help if the report sorted the types by their fully qualified name. 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. STOB.canBeInstantiated should lose its logLevel parameter, because it's no longer used. Yay! I didn't scan carefully, but perhaps some other methods could lose a similar parameter. Maybe even some loggers can go away. Likewise, you HAVE to delete the isSpeculative parameter from all methods that have one. This is this patch's greatest accomplishment, and is why it generates such better error messages. You should delete it, revive it, and delete it again, and then upload the video to YouTube. I see four calls still to getLogLevel(), but it looks like all of them should go away in favor of recording into ProblemReport. STOB.build needs formatting Style things, up to you: The first variant of shouldConsiderFieldsForSerialization invites callers to discard problem messages. IMHO, delete it and force people to supply an empty list if they really want to ignore the problems. Returning an empty list from getProblemsForType looks safer and more convenient than returning null. It's as easy to call isEmpty() as to check for null, and there are many idioms where having an empty list is more convenient than having a list-or-null. -Lex --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
