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.schu...@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@freenetproject.org >> https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl > > _______________________________________________ > Devl mailing list > Devl@freenetproject.org > https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl _______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
