Hmm, I was under the assumption that synchronized blocks locked variables, not objects. Seems I have to review a few things...
Regarding making Log instantiable, is that really necessary? IMHO, TID filtering is a better way to handle multi-node simulations when, and if that becomes necessary. There's also the fact that all classes using logging facilities would need to be _manually_ tweaked. The prime reason the change between old and proposed APIs is feasible is due to the fact that most of the work can be done almost automatically (~8000 lines touched by a sed script + ~100 manually tweaked lines), and, because the sed script is short, it can easily be verified, and its results replicated. On 27-03-2012 21:04, Zlatin Balevsky wrote: > There are many problems with > https://github.com/Heiral/fred-staging/commit/b876f8c454cb269281ffcb18d14d25b22818d130#L0R49 > : > > > line 25: the static field log should be volatile. It may work now and > then but it's broken. Google "double-checked locking" for more info. > Either that or synchronize it properly everywhere > > lines 49-60 - "out" will refer to the parameter, not the static field > so you will end up closing the parameter. Also what if Log.out is > null? Can't synchronize on null. > > Line 61: bad synchronization again > > lines 65-75 Level.ordinal() will compile to a vtable call if you have > more than two levels in use across the jvm. > > lines 100-101: again, if out is null you can't synchronize on it. > > > Besides all this, adding a static dependency will create problems > if/when a multi-node simulator or multi-node black box unit tests are > written. > > On Tue, Mar 27, 2012 at 2:35 PM, Marco Schulze > <marco.c.schulze at gmail.com> wrote: >> On 27-03-2012 12:51, Martin Nyhus wrote: >>> On Monday 26. March 2012 00:22:24 Marco Schulze wrote: >>>> Working (but incomplete) code is available @ >>>> https://github.com/Heiral/fred-staging/tree/logger++ >>> Please keep in mind that simply deleting Logger will break pretty much >>> every >>> single plugin out there, so you really should rewrite Logger to call the >>> new >>> methods in your new logger class. >> Will do. >> >> >>> I won't say much about the code since you say you aren't finished, but >>> please >>> follow the code style of the rest of the code base. >> Apart from the lack of braces, what violates the coding standards? I mean, >> compared to the rest of fred code, I use too many blank lines, 80 character >> lines, variables are declared at the top of the function and other minor >> details. I hope that that isn't a problem. >> >> >>> Also, I'm pretty sure you don't want to close the new stream here[0]. >> Fixed. >> >> >>> And the >>> locking when you update out isn't sufficient for visibility (either that >>> or it >>> isn't clear enough why it works IMHO). >> You mean, things like if(out == null) without locking (inside isLoggable())? >> In this case, it doesn't really matter as it is meant to be a very quick >> check and getting the wrong values don't matter much. >> >> >>> [0] https://github.com/Heiral/fred- >>> staging/commit/b876f8c454cb269281ffcb18d14d25b22818d130#L0R49 >>> _______________________________________________ >>> Devl mailing list >>> Devl at freenetproject.org >>> https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl >> _______________________________________________ >> Devl mailing list >> Devl at freenetproject.org >> https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl > _______________________________________________ > Devl mailing list > Devl at freenetproject.org > https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
