You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@stdcxx.apache.org by Martin Sebor <se...@roguewave.com> on 2008/06/06 17:35:13 UTC
Re: svn commit: r663377 - in /stdcxx/branches/4.2.x: include/valarray
src/valarray.cpp tests/numerics/26.class.gslice.cpp tests/regress/26.valarray.sub.stdcxx-955.cpp
vitek@apache.org wrote:
> Author: vitek
> Date: Wed Jun 4 14:48:36 2008
> New Revision: 663377
>
[...]
> Modified: stdcxx/branches/4.2.x/src/valarray.cpp
> URL: http://svn.apache.org/viewvc/stdcxx/branches/4.2.x/src/valarray.cpp?rev=663377&r1=663376&r2=663377&view=diff
> ==============================================================================
> --- stdcxx/branches/4.2.x/src/valarray.cpp (original)
> +++ stdcxx/branches/4.2.x/src/valarray.cpp Wed Jun 4 14:48:36 2008
> @@ -41,8 +41,12 @@
> {
> _RWSTD_SIZE_T __n = _C_length.size ();
>
> - while (__n && _C_r_length [__n - 1] == _C_length [__n - 1] - 1)
> - --__n;
> + for (/**/; __n; --__n)
> + {
The brace should be on the line above :)
> + if ( _C_length [__n - 1]
> + && _C_r_length [__n - 1] != _C_length [__n - 1] - 1)
Also, I wonder if it might help generate more optimal code to write
the loop like so:
while (__n) {
--__n;
if (_C_length [n] && _C_r_length [n] != _C_length [n] - 1)
break;
}
The duplicate check for (0 == n) below could probably be hoisted
into the loop for even more optimal code, something like this:
for ( ; ; ) {
if (0 == n) {
_C_reset = true;
break;
}
--n;
if (_C_length [n] && _C_r_length [n] != _C_length [n] - 1)
break;
}
> + break;
> + }
>
> if (0 == __n) {
> _C_reset = true;
>
[...]
> Added: stdcxx/branches/4.2.x/tests/regress/26.valarray.sub.stdcxx-955.cpp
> URL: http://svn.apache.org/viewvc/stdcxx/branches/4.2.x/tests/regress/26.valarray.sub.stdcxx-955.cpp?rev=663377&view=auto
> ==============================================================================
> --- stdcxx/branches/4.2.x/tests/regress/26.valarray.sub.stdcxx-955.cpp (added)
> +++ stdcxx/branches/4.2.x/tests/regress/26.valarray.sub.stdcxx-955.cpp Wed Jun 4 14:48:36 2008
> @@ -0,0 +1,59 @@
> +/************************************************************************
> + *
> + * 26.valarray.sub.stdcxx-955.cpp - regression test for STDCXX-955
> + *
> + * http://issues.apache.org/jira/browse/STDCXX-955
> + *
> + * $Id$
> + *
> + ***************************************************************************
> + *
> + * 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.
> + *
> + * Copyright 1994-2008 Rogue Wave Software, Inc.
^^^^
Maybe we should just drop the copyright in new code?
Martin
Re: svn commit: r663377 - in /stdcxx/branches/4.2.x: include/valarray
src/valarray.cpp tests/numerics/26.class.gslice.cpp tests/regress/26.valarray.sub.stdcxx-955.cpp
Posted by Martin Sebor <se...@roguewave.com>.
Travis Vitek wrote:
>
>
> Martin Sebor wrote:
>> vitek@apache.org wrote:
>>> Author: vitek
>>> Date: Wed Jun 4 14:48:36 2008
>>> New Revision: 663377
>>>
>> [...]
>>> Modified: stdcxx/branches/4.2.x/src/valarray.cpp
>>> URL:
>> http://svn.apache.org/viewvc/stdcxx/branches/4.2.x/src/valarray
>> .cpp?rev=663377&r1=663376&r2=663377&view=diff
>> ===============================================================
>> ===============
>>> --- stdcxx/branches/4.2.x/src/valarray.cpp (original)
>>> +++ stdcxx/branches/4.2.x/src/valarray.cpp Wed Jun 4 14:48:36 2008
>>> @@ -41,8 +41,12 @@
>>> {
>>> _RWSTD_SIZE_T __n = _C_length.size ();
>>>
>>> - while (__n && _C_r_length [__n - 1] == _C_length [__n - 1] - 1)
>>> - --__n;
>>> + for (/**/; __n; --__n)
>>> + {
>> The brace should be on the line above :)
>>
>
> Blarg!
>
>>> + if ( _C_length [__n - 1]
>>> + && _C_r_length [__n - 1] != _C_length [__n - 1] - 1)
>> Also, I wonder if it might help generate more optimal code to write
>> the loop like so:
>>
>> while (__n) {
>> --__n;
>>
>> if (_C_length [n] && _C_r_length [n] != _C_length [n] - 1)
>> break;
>> }
>>
>
> It is possible it would help generate better code, but this code changes
> the behavior of the loop. We now decrement __n before checking the
> condition; when we break __n will be off-by-one. Obviously we can deal
> with that by incrementing __n before the break.
I should have looked at the rest of the file and not just the diff
before saying anything.
>
> I'll take a quick look at some disassembly and decide if it is worth
> making the change or not.
It probably doesn't matter in the grand scheme of things. The new
gslice computes the indices just once and caches the result which
should be a much more significant optimization.
Martin
RE: svn commit: r663377 - in /stdcxx/branches/4.2.x: include/valarray src/valarray.cpp tests/numerics/26.class.gslice.cpp tests/regress/26.valarray.sub.stdcxx-955.cpp
Posted by Travis Vitek <Tr...@roguewave.com>.
Martin Sebor wrote:
>
>vitek@apache.org wrote:
>> Author: vitek
>> Date: Wed Jun 4 14:48:36 2008
>> New Revision: 663377
>>
>[...]
>> Modified: stdcxx/branches/4.2.x/src/valarray.cpp
>> URL:
>http://svn.apache.org/viewvc/stdcxx/branches/4.2.x/src/valarray
>.cpp?rev=663377&r1=663376&r2=663377&view=diff
>>
>===============================================================
>===============
>> --- stdcxx/branches/4.2.x/src/valarray.cpp (original)
>> +++ stdcxx/branches/4.2.x/src/valarray.cpp Wed Jun 4 14:48:36 2008
>> @@ -41,8 +41,12 @@
>> {
>> _RWSTD_SIZE_T __n = _C_length.size ();
>>
>> - while (__n && _C_r_length [__n - 1] == _C_length [__n - 1] - 1)
>> - --__n;
>> + for (/**/; __n; --__n)
>> + {
>
>The brace should be on the line above :)
>
Blarg!
>> + if ( _C_length [__n - 1]
>> + && _C_r_length [__n - 1] != _C_length [__n - 1] - 1)
>
>Also, I wonder if it might help generate more optimal code to write
>the loop like so:
>
> while (__n) {
> --__n;
>
> if (_C_length [n] && _C_r_length [n] != _C_length [n] - 1)
> break;
> }
>
It is possible it would help generate better code, but this code changes
the behavior of the loop. We now decrement __n before checking the
condition; when we break __n will be off-by-one. Obviously we can deal
with that by incrementing __n before the break.
I'll take a quick look at some disassembly and decide if it is worth
making the change or not.
>The duplicate check for (0 == n) below could probably be hoisted
>into the loop for even more optimal code, something like this:
>
> for ( ; ; ) {
> if (0 == n) {
> _C_reset = true;
> break;
> }
>
> --n;
>
> if (_C_length [n] && _C_r_length [n] != _C_length [n] - 1)
> break;
> }
>
>> + break;
>> + }
>>
>> if (0 == __n) {
>> _C_reset = true;
>>