diff mbox series

Patch: Remove unneeded double operation in libstdc++-v3/src/c++17/fs_path.cc

Message ID 878r5r99or.fsf@calavera
State Not Applicable
Headers
Series Patch: Remove unneeded double operation in libstdc++-v3/src/c++17/fs_path.cc |

Checks

Context Check Description
snail/gcc-patch-check fail Git am fail log

Commit Message

Martin Küttler Dec. 18, 2023, 8:36 a.m. UTC 
  This is a small change to libstdc++ which does not change any behavior.

This change has two, ihmo positive, implications:

 - The implicit conversion from double to int is avoided (Avoiding a
   warning).

 - No floating point number is used at all, which could be significant
   in some scenarios.

------------

Comments

Jonathan Wakely Jan. 5, 2024, 12:41 p.m. UTC | #1 
On 18/12/23 09:36 +0100, Martin Küttler wrote:
>This is a small change to libstdc++ which does not change any behavior.

Please CC the libstdc++@gcc.gnu.org list on all libstdc++ patches, as
documented at https://gcc.gnu.org/lists.html

Otherwise I won't see the patches unless I happen to glance at the
gcc-patches archive by chance.

>This change has two, ihmo positive, implications:
>
> - The implicit conversion from double to int is avoided (Avoiding a
>   warning).

I don't see any warning here. What do you see?

> - No floating point number is used at all, which could be significant
>   in some scenarios.

Yes, it seems worth doing for this reason. I'll test+push the patch,
thanks.

Looking at path::_List::reserve now, we probably also want to avoid
overflow. Although a path with INT_MAX/1.5 components seems
implausible for 32-bit and 64-bit targets, it could be a problem for
16-bit targets. I'll take care of that too.

>------------
>
>diff --git a/libstdc++-v3/src/c++17/fs_path.cc b/libstdc++-v3/src/c++17/fs_path.cc
>index d65b5482e8b..b47ed0aa7aa 100644
>--- a/libstdc++-v3/src/c++17/fs_path.cc
>+++ b/libstdc++-v3/src/c++17/fs_path.cc
>@@ -447,8 +447,9 @@ path::_List::reserve(int newcap, bool exact = false)
>
>   if (curcap < newcap)
>     {
>-      if (!exact && newcap < int(1.5 * curcap))
>-       newcap = 1.5 * curcap;
>+      const int nextcap = curcap + curcap / 2;
>+      if (!exact && newcap < nextcap)
>+       newcap = nextcap;
>
>       void* p = ::operator new(sizeof(_Impl) + newcap * sizeof(value_type));
>       std::unique_ptr<_Impl, _Impl_deleter> newptr(::new(p) _Impl{newcap});
Martin Küttler Jan. 5, 2024, 12:45 p.m. UTC | #2 
>>This is a small change to libstdc++ which does not change any behavior.
>
> Please CC the libstdc++@gcc.gnu.org list on all libstdc++ patches, as
> documented at https://gcc.gnu.org/lists.html

Acknowledged. Sorry.

>>This change has two, ihmo positive, implications:
>>
>> - The implicit conversion from double to int is avoided (Avoiding a
>>   warning).
>
> I don't see any warning here. What do you see?

I see "warning: conversion from ‘double’ to ‘int’ may change value
[-Wfloat-conversion]" This appears to be a specifically enabled warning.

> Looking at path::_List::reserve now, we probably also want to avoid
> overflow. Although a path with INT_MAX/1.5 components seems
> implausible for 32-bit and 64-bit targets, it could be a problem for
> 16-bit targets. I'll take care of that too.

Nice catch.

Martin

--
Kernkonzept GmbH at Dresden, Germany, HRB 31129, CEO Dr.-Ing. Michael Hohmuth
Jonathan Wakely Jan. 5, 2024, 1:02 p.m. UTC | #3 
On Fri, 5 Jan 2024 at 13:00, Martin Küttler
<martin.kuettler@kernkonzept.com> wrote:
>
>
> >>This is a small change to libstdc++ which does not change any behavior.
> >
> > Please CC the libstdc++@gcc.gnu.org list on all libstdc++ patches, as
> > documented at https://gcc.gnu.org/lists.html
>
> Acknowledged. Sorry.
>
> >>This change has two, ihmo positive, implications:
> >>
> >> - The implicit conversion from double to int is avoided (Avoiding a
> >>   warning).
> >
> > I don't see any warning here. What do you see?
>
> I see "warning: conversion from ‘double’ to ‘int’ may change value
> [-Wfloat-conversion]" This appears to be a specifically enabled warning.
>
> > Looking at path::_List::reserve now, we probably also want to avoid
> > overflow. Although a path with INT_MAX/1.5 components seems
> > implausible for 32-bit and 64-bit targets, it could be a problem for
> > 16-bit targets. I'll take care of that too.
>
> Nice catch.


We also have some redundant code in path::operator/= which can just be
removed, because _List::reserve does it anyway:

  if (orig_type == _Type::_Multi)
    {
      const int curcap = _M_cmpts._M_impl->capacity();
      if (capacity > curcap)
        capacity = std::max(capacity, (int) (curcap * 1.5));
    }
diff mbox series

Patch

diff --git a/libstdc++-v3/src/c++17/fs_path.cc b/libstdc++-v3/src/c++17/fs_path.cc
index d65b5482e8b..b47ed0aa7aa 100644
--- a/libstdc++-v3/src/c++17/fs_path.cc
+++ b/libstdc++-v3/src/c++17/fs_path.cc
@@ -447,8 +447,9 @@  path::_List::reserve(int newcap, bool exact = false)

   if (curcap < newcap)
     {
-      if (!exact && newcap < int(1.5 * curcap))
-       newcap = 1.5 * curcap;
+      const int nextcap = curcap + curcap / 2;
+      if (!exact && newcap < nextcap)
+       newcap = nextcap;

       void* p = ::operator new(sizeof(_Impl) + newcap * sizeof(value_type));
       std::unique_ptr<_Impl, _Impl_deleter> newptr(::new(p) _Impl{newcap});