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: svn commit: r801484 - HARMONY-6279 file.encoding improvement
Date Thu, 06 Aug 2009 10:35:04 GMT

Thanks Charles.  I've made those changes.  What about the more important
'inline' issue?

Regards,
 Mark.

In message <5948b71e0908060209g2c844f83sc2a920c1ddfe3c2f@mail.gmail.com>,
> Charles Lee writes:
>
> On Thu, Aug 6, 2009 at 4:54 PM, Mark Hindess <mark.hindess@googlemail.com>
> wrote:
> 
> > Charles, Regis,
> >
> > Some comments regarding this commit...
> >
> > In message <20090806010230.9DD53238887A@eris.apache.org>,
> > regisxu@apache.org
> > writes:
> > >
> > > Author: regisxu
> > > Date: Thu Aug  6 01:02:29 2009
> > > New Revision: 801484
> > >
> > > URL: http://svn.apache.org/viewvc?rev=801484&view=rev
> > > Log:
> > > Apply patch for HARMONY-6279: [classlib][luni] file.encoding is always
> > set to
> > >  ISO-9959-1 if using drlvm
> >
> > > [snip]
> >
> > > Modified: modules/luni/src/main/native/luni/unix/helpers.c
> > >
> > ===========================================================================
> ==
> > > --- modules/luni/src/main/native/luni/unix/helpers.c (original)
> > > +++ modules/luni/src/main/native/luni/unix/helpers.c Thu Aug  6 01:02:29
> > 2009
> > > @@ -220,3 +220,41 @@
> > >
> > >    hysock_setopt_bool (socketP, HY_SOL_SOCKET, HY_SO_REUSEADDR, &value);
> > >  }
> > > +
> > > +/* Get charset from the OS */
> > > +inline void getOSCharset(char *locale, const size_t size) {
> > > +  char * codec = NULL;
> > > +  size_t cur = 0;
> > > +  short flag = 0;
> > > +  setlocale(LC_CTYPE, "");
> > > +  codec = setlocale(LC_CTYPE, NULL);
> > > +  // get codeset from language[_territory][.codeset][@modifier]
> > > +  while (*codec) {
> > > +    if (!flag) {
> > > +      if (*codec != '.') {
> > > +        codec++;
> > > +        continue;
> > > +      } else {
> > > +        flag = 1;
> > > +        codec++;
> > > +      }
> > > +    } else {
> > > +      if (*codec == '@') {
> > > +        break;
> > > +      } else {
> > > +        locale[cur++] = (*codec);
> > > +        codec++;
> > > +        if (cur >= size) {
> > > +          // Not enough size
> > > +          cur = 0;
> > > +          break;
> > > +        }
> > > +      }
> > > +    }
> > > +  }
> > > +  locale[cur] = '\0';
> > > +  if (!strlen(locale)) {
> > > +    strcpy(locale, "8859_1");
> > > +  }
> > > +  return;
> > > +}
> >
> > The inline keyword causes a compiler error on AIX:
> >
> >  "helpers.c", line 225.1: 1506-485 (S) Parameter declaration list is
> >  incompatible with declarator for inline.
> >
> > I was going to #ifdef out the inline for AIX.  However, I'm not
> > convinced inline makes sense here even for Linux.  In -Dhy.cfg=debug
> > mode there are no optimizations so this function is never inlined, but
> > even in -Dhy.cfg=release mode none of the gcc versions I tried inlined
> > this function.
> >
> > So, I see no benefit from having it.  I suggest we remove it for
> > portability.  Or is there a benefit that I am missing?
> >
> > > Added: modules/luni/src/main/native/luni/windows/charsetmap.h
> > > --- modules/luni/src/main/native/luni/windows/charsetmap.h (added)
> > > +++ modules/luni/src/main/native/luni/windowscharsetmap.h Thu Aug  6
> > 01:02:29
> > > @@ -0,0 +1,123 @@
> > > +/*
> > > + *  Licensed to the Apache Software Foundation (ASF) under one or more
> > > + *  contributor license agreements.  See the NOTICE file distributed
> > with
> > > + *  this work for additional information regarding copyright ownership.
> > > + *  The ASF licenses this file to You under the Apache License, Version
> > 2.0
> > > + *  (the "License"); you may not use this file except in compliance with
> > > + *  the License.  You may obtain a copy of the License at
> > > + *
> > > + *     http://www.apache.org/licenses/LICENSE-2.0
> > > + *
> > > + *  Unless required by applicable law or agreed to in writing, software
> > > + *  distributed under the License is distributed on an "AS IS" BASIS,
> > > + *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> > implied.
> > > + *  See the License for the specific language governing permissions and
> > > + *  limitations under the License.
> > > + */
> > > +/**
> > > + * @author Charles Lee
> > > + * @version $Revision: 1.0$
> > > + */
> >
> > What is this Revision?  I don't think it makes sense in this context.
> >
> > I notice that there are '$Revision...' tags in over 3k files in classlib
> > alone but since we don't have svn keyword expansion enabled (and I hope
> > we never do) then most are either not expanded or they were expanded
> > elsewhere and are just misleading in the context of our svn repository.
> > I suggest we remove them?
> >
> > (I'd rather not see author tags either but I'm not sure everyone agrees
> > with this.  There is some discussion in the list archives.)
> 
> 
> I'm OK. Please delete it.
> 
> >
> >
> > > [snip]
> >
> > > Modified: modules/luni/src/main/native/luni/windows/helpers.c
> > > --- modules/luni/src/main/native/luni/windows/helpers.c (original)
> > > +++ modules/luni/src/main/native/luni/windows/helpers.c Thu Aug  6
> > 01:02:29 2009
> > > @@ -25,12 +25,14 @@
> > >  #include <windows.h>
> > >  #include <winbase.h>
> > >  #include <stdlib.h>
> > > +#include <stdio.h>
> > >  #include <LMCONS.H>
> > >  #include <direct.h>
> >
> > I don't have a windows machine configured to build Harmony right now but I
> > suspect that stdio.h was only required for your debugging and can/should
> > be removed from any patches/commits?
> 
> 
> I have compiled it on the windows. Seems I forget to delete it when deleted
> my debug function. Please remove it. Thanks.
> 
> >
> > Regards,
> >  Mark.
> -- 
> Yours sincerely,
> Charles Lee



Mime
View raw message