harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Hindess <mark.hind...@googlemail.com>
Subject svn commit: r801484 - HARMONY-6279 file.encoding improvement
Date Thu, 06 Aug 2009 08:54:53 GMT
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.)

> [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?

Regards,
 Mark.



Mime
View raw message