You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Mark Hindess <ma...@googlemail.com> on 2009/08/06 10:54:53 UTC
svn commit: r801484 - HARMONY-6279 file.encoding improvement
Charles, Regis,
Some comments regarding this commit...
In message <20...@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.
Re: svn commit: r801484 - HARMONY-6279 file.encoding improvement
Posted by Oliver Deakin <ol...@googlemail.com>.
Mark Hindess wrote:
> <SNIP!>
> 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?
>
+1 to removing the revision tags - I don't think they are any use at the
moment, not just because they don't get expanded but also because they
don't cover all files.
Regards,
Oliver
> (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.
>
>
>
>
--
Oliver Deakin
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Re: svn commit: r801484 - HARMONY-6279 file.encoding improvement
Posted by Charles Lee <li...@gmail.com>.
Hi Mark,
I add inline just want it to be what it supposed to be. Since it is just a
hint, remove it will be ok.
By the way, why can't AIX recognize the 'inline'?
On Thu, Aug 6, 2009 at 6:35 PM, Mark Hindess <ma...@googlemail.com>wrote:
>
> Thanks Charles. I've made those changes. What about the more important
> 'inline' issue?
>
> Regards,
> Mark.
>
> In message <59...@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 <20...@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
>
>
>
--
Yours sincerely,
Charles Lee
Re: svn commit: r801484 - HARMONY-6279 file.encoding improvement
Posted by Mark Hindess <ma...@googlemail.com>.
Thanks Charles. I've made those changes. What about the more important
'inline' issue?
Regards,
Mark.
In message <59...@mail.gmail.com>,
> Charles Lee writes:
>
> On Thu, Aug 6, 2009 at 4:54 PM, Mark Hindess <ma...@googlemail.com>
> wrote:
>
> > Charles, Regis,
> >
> > Some comments regarding this commit...
> >
> > In message <20...@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
Re: svn commit: r801484 - HARMONY-6279 file.encoding improvement
Posted by Charles Lee <li...@gmail.com>.
On Thu, Aug 6, 2009 at 4:54 PM, Mark Hindess <ma...@googlemail.com>wrote:
> Charles, Regis,
>
> Some comments regarding this commit...
>
> In message <20...@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