harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Hindess <mark.hind...@googlemail.com>
Subject Re: [general] icu or apr-iconv, which coding library is better?
Date Wed, 16 Apr 2008 11:38:42 GMT


Please don't take any of my comments as a reason not to commit the patch
as is.  None of the issues I have should stop it being committed.  (Well
the execute permission one might be worth fixing but I'm sure Alexey can
do that without a new patch.)

On 16 April 2008 at 14:02, "Alexei Fedotov" <alexei.fedotov@gmail.com> wrote:
> 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.

No, don't worry.  I am sure they are reasonalbe.  I was just a little
curious as they were not obviously related to the topic of the JIRA.

> 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.

That sounds like a reasonable policy but you don't seem to be applying it
consistently. ;-)  For example:

--- vm/vmstart/src/compmgr/component_manager_impl.cpp   (revision 648214)
+++ vm/vmstart/src/compmgr/component_manager_impl.cpp   (working copy)
@@ -16,15 +16,14 @@
  * @author Alexei Fedotov
- * @version $Revision: $


--- vm/interpreter/src/interpreter_ti.cpp       (revision 648214)
+++ vm/interpreter/src/interpreter_ti.cpp       (working copy)
@@ -14,10 +14,6 @@
  *  See the License for the specific language governing permissions and
  *  limitations under the License.
- * @author Ivan Volosyuk
- * @version $Revision: $
- */  

There are several other instances where you don't remove your own name
or where you do remove the names of others.

I don't really care except it clutters up the patch and makes it
difficult to see the significant changes.

IMNSHO, It would be good to get consensus and (if agreed) remove all of
these in one go.  Certainly I can't see a good argument for keeping the
@version tags.

> 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.

I agree.  I was just making the comment since you used lines as a metric
in one of the JIRA comments.  ;-)

> 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.

I guess it is personal preference but when I come across similar issues
then I tend to use something like quilt to make a series of patches.  So
that the cleanup can be kept separately - and applied even if someone
objects to the functional changes.  It would also have meant that on the
JIRA you'd have one (or maybe two versions) of the cleanup patch and
several iterations of a much smaller (easier to read/review) patch with
the significant changes.

Personally, I'd be happy to see more trivial JIRA that are just fixing
inconsistencies than see these inconsistencies cluttering up other
functional patches.

> 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).

The threading code was already bothering me for other reasons so I guess
I overlooked that.  For instance, the line from vm/thread/src/thread_native_thi
n_monitor.c that reads:

  CTRACE(("try lock %x count:%d", this_id, res_lock_count++));

with the side-effect of incrementing res_lock_count only if tracing is
puzzled me a little until I realised that this variable was only used
by the trace call.  (Note, these trace-only variables should only be
declared if tracing is enabled so that we avoid the extra footprint and
potential compiler warnings about them being declared but not used.)

Finally, don't let any of the comments detract from the good work you've
done.  I do have a great deal of sympathy for someone who likes to clean
up as they work but for reasonably large patches with (potentially) contentious
changes it seems a shame not to keep the cleanup parts separately so:

a) they can be applied sooner, and
b) they are not lost if the contentious parts are rejected


> 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 observation
> s:
> >
> >  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_cont
> ext
> >  static.
> >
> >  3) Why were @version and @author tags removed from some files and only @ve
> rsion
> >  tags from others?  Some of these were removed from files that were otherwi
> se
> >  untouched so cynical people might think someone was just trying to make th
> e
> >  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

View raw message