Mailing-List: contact dev-help@harmony.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@harmony.apache.org
Received-SPF: pass (athena.apache.org: domain of alexei.fedotov@gmail.com
 designates 209.85.146.183 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=message-id:date:from:to:subject:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references;
        b=Z1MeRnv8m2AGiGuCOGNO7i9awqo/uwNvdY7T3lGiMgIxpZBRn/GKWK1QlnoBG17UTrstY0n9GkVEJQvIk7bJxhyAf5NK1a90Znfj4QlY09/ZmJ2g+Arn5yJP5pxabo+ZOVe6J1QSUbvVze8sFjTGkJ2Peyk5tl0uDAaRUvkxZ8Q=
Message-ID: <c30d70960804160302l223012dfqa60a85e149b91a88@mail.gmail.com>
Date: Wed, 16 Apr 2008 14:02:33 +0400
From: "Alexei Fedotov" <alexei.fedotov@gmail.com>
To: dev@harmony.apache.org
Subject: Re: [general] icu or apr-iconv, which coding library is better?
In-Reply-To: <200804160907.m3G97fpp030394@d12av01.megacenter.de.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
References: <c30d70960804112037g4e704fe8mf459554393f39fe9@mail.gmail.com>
	 <3b3f27c60804121330v4a30a1f9rf7662e017bc9c975@mail.gmail.com>
	 <c30d70960804130217w7ecfeb3ag545358e3e54ac0c6@mail.gmail.com>
	 <3b3f27c60804131939k381c403fg5ec084b667c094f3@mail.gmail.com>
	 <c30d70960804132126s35cb22a5y976c854e35ca3a19@mail.gmail.com>
	 <c30d70960804150340o575737e5kb9f9da25f8145662@mail.gmail.com>
	 <3b3f27c60804151900y7af29c15n93fcf7a578d0ed22@mail.gmail.com>
	 <f944d83d0804152145q54659c89j921232a789af22b8@mail.gmail.com>
	 <200804160907.m3G97fpp030394@d12av01.megacenter.de.ibm.com>

Mark,
I'm impressed with your approach.

0) I don't know why Mikhail Fursov choose that style of logging. I
just renamed functions keeping the code as is. If Mikhail does not
object, I will change it.

1) Thanks for catching it. Fixed that once, then forgot.

2) I've got a problem on Windows 64 platform related to this function
and tried to fix this to pass linking. I was only able to build
incrementally on that platform due to some infrastructure reasons.
Generally, the more statics, the better. Can separate it in another
patch easily.

3) This is tricky. Few things are involved.
3a) I generally remove this header from files, where I fix other headers.
3b) I don't touch active people names except myself and Pavel Pervov
from whom I get a commitment that it's ok.
3c) Generally, decrease in lines is always positive. Even if these
lines are comments which are not useful.
3d) I think that if one adds changes suggested by Alexey, the size of
the logger would be equal to the wrapper size. So please just don't
use lines as a proper metrics. The main benefit is removing libraries,
which contain more source files than our whole vmcore.

4a)
-Pavel, how should I merge in this #if 0?
-Just remove it.

4b) The same thing as for names. When I come to the incorrect place, I
just fix it. Having hanging thread safety issues in Mikhail verifier,
building that verifier with VC6 and trying to finalize CDS I cannot
allow myself to go and clean up the code separately. Ok, you may say:
in this case do not make a clean up at all. But I'm getting fun when
making code look pretty, and cosmetic changes help me merging my brain
into code before doing serious changes.

Thanks for a very careful review. You caught all places except one
which concerned me. (The last one is logging for hythread which is
actually is done by means of print. This is evolutionary improvement).

On Wed, Apr 16, 2008 at 1:07 PM, Mark Hindess
<mark.hindess@googlemail.com> wrote:
>
>  On 16 April 2008 at 11:45, "Alexey Varlamov" <alexey.v.varlamov@gmail.com>
>  wrote:
>
> > 2008/4/16, Nathan Beyer <nbeyer@gmail.com>:
>  > > I think we need more consensus on this. Is everyone in agreement about
>  > > dumping log4cxx? Is this patch the appropriate replacement?
>  >
>  > Well, the suggestion has positive responses from me, Xiao-Feng and
>  > Ilya Berezhniuk, and no specifc opposing arguments. Let's wait last
>  > 24h, tomorrow I'll commit if no objections.
>
>  +1 from me; making the logging more consistent is a big improvement.
>  Nice work Alexei.
>
>  I see no reason not to apply it as is but do have some trivial observations:
>
>  0) I'd probably make the change to vm/em/src/EBProfileCollector.cpp:
>
>  -    loggingEnabled =  is_info_enabled(LOG_DOMAIN);
>  -    if (!loggingEnabled) {
>  -        loggingEnabled = is_info_enabled(catName.c_str());
>  -    }
>  +    loggingEnabled =  log_is_info_enabled(LOG_DOMAIN) ||
>  log_is_info_enabled(catName.c_str());
>
>  to be consistent with a couple of similar chunks of code.
>
>  1) There is no reason for:
>
>   Property changes on: vm/port/src/logger/logparams.cpp
>   ___________________________________________________________________
>   Name: svn:executable
>      + *
>
>  2) It is hard to see the relevance of some changes.  Such as adding the:
>
>   #include "open/hythread.h"
>
>  to vm/vmcore/include/stack_trace.h or making native_get_regs_from_jit_context
>  static.
>
>  3) Why were @version and @author tags removed from some files and only @version
>  tags from others?  Some of these were removed from files that were otherwise
>  untouched so cynical people might think someone was just trying to make the
>  number of '-' lines in the patch look "better". ;-)
>
>  4) There were lots of spurious changes that could have been made in a
>  trivial cleanup patch to keep the important changes more transparent.
>  Things like removing @version tags, renaming pool1 back to pool, and
>  changes like:
>
>   -#ifndef _VMCORE_LOG_H_
>   -#define _VMCORE_LOG_H_
>   +#ifndef _VMCORE_LOG_H
>   +#define _VMCORE_LOG_H
>
>  and removing the '#if 0' block from vm/vmcore/src/class_support/class_impl.cpp.
>
>  -Mark.
>
>
>


-- 
With best regards,
Alexei