harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Charles Lee <littlee1...@gmail.com>
Subject Re: svn commit: r801484 - HARMONY-6279 file.encoding improvement
Date Thu, 06 Aug 2009 09:09:56 GMT
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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message