Re: [Patch] Implementation of n3793
On 2013-11-01 21:46, Paolo Carlini wrote: Hi, Il giorno 01/nov/2013, alle ore 21:09, Jonathan Wakely ha scritto: Here's the final version of Luc's optional implementation that I'm committing, tested on x86_64-linux. Great. Just noticed a minor nit: the fallback __constexpr_addressof appears not to be inline. Paolo Can you expand? I think it's just as much inline as the other overload -- does it need to be different?
Re: [Patch] Implementation of n3793
On 2013-11-02 19:02, Paolo Carlini wrote: Can you expand? I think it's just as much inline as the other overload -- does it need to be different? The other overload is constexpr thus it's implicitly inline. The fall back is very simple too and I think it should be declared inline, unless you analyzed the assembly and believe it normally boils down to more than, say, 5 instructions. Paolo I see. It didn't occur to me to declare it inline, as I only ever use the keyword to satisfy ODR requirements. E.g. the non-member swap isn't declared inline either. For future reference, is there a rule of thumb in use?
Re: [Patch] Implementation of n3793
On 2013-11-02 23:31, Paolo Carlini wrote:
In general we are very careful with code bloat, but free functions which just
forward to other functions should be definiyely inline, otherwise typically at
widely used optimization levels like -O2 users get suboptimal performance for
no reason. But please feel free to experiment and report your findings on the
mailing list!
Paolo
After review, the free functions that only ever forward to something
else and are not implicitly inline appear to be that
__constexpr_addressof overload and non-member swap. Here's a patch
(against master) that marks them inline, all tests pass. I briefly and
casually attempted to notice any effect, but to be honest in my smallish
testcases GCC appeared to already see through everything.
>From 0175e61f3295159fe7ac2fefec05b829ecd6bc4d Mon Sep 17 00:00:00 2001
From: Michael Brune
Date: Sun, 3 Nov 2013 01:37:16 +0100
Subject: [PATCH] Marked some forwarding free functions as inline that weren't
already.
---
libstdc++-v3/include/experimental/optional | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libstdc++-v3/include/experimental/optional b/libstdc++-v3/include/experimental/optional
index 5915892..06e0f29 100644
--- a/libstdc++-v3/include/experimental/optional
+++ b/libstdc++-v3/include/experimental/optional
@@ -157,7 +157,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
*/
template::value,
int>::type...>
-_Tp* __constexpr_addressof(_Tp& __t)
+inline _Tp* __constexpr_addressof(_Tp& __t)
{ return std::__addressof(__t); }
/**
@@ -790,7 +790,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// [X.Y.11]
template
-void
+inline void
swap(optional<_Tp>& __lhs, optional<_Tp>& __rhs)
noexcept(noexcept(__lhs.swap(__rhs)))
{ __lhs.swap(__rhs); }
--
1.8.1.2
Re: [Patch] Implementation of n3793
On 2013-11-03 12:19, Jonathan Wakely wrote:
On 3 November 2013 09:49, Andreas Schwab wrote:
Jonathan Wakely writes:
+ {
+std::experimental::optional o { std::experimental::in_place,
0x1234ABCDF1E2D3C4 };
+auto copy = o;
+VERIFY( copy );
+VERIFY( *copy == 0x1234ABCDF1E2D3C4 );
+VERIFY( o && o == 0x1234ABCDF1E2D3C4 );
+ }
experimental/optional/cons/copy.cc:70:20: error: no match for 'operator==' (operand
types are 'std::experimental::optional' and 'long long int')
VERIFY( o && o == 0x1234ABCDF1E2D3C4 );
^
Yes, Paolo pointed out these are failing on 32-bit targets, I've got a
patch coming. Luc is there any reason not to just replace all those
large constants with 0x1234ABCD, which fits in a long on 32-bit
targets?
The value is arbitrary, it's fine to proceed.
Re: [PATCH] Define std::to_chars and std::from_chars for C++17 (P0067R5, partial)
On 07/04/17 13:29 +0100, Jonathan Wakely wrote:
+ constexpr char __abc[] = "abcdefghijklmnopqrstuvwxyz";
+ unsigned char __lc = std::tolower(__c);
+ constexpr bool __consecutive = __consecutive_chars(__abc, 26);
+ if _GLIBCXX17_CONSTEXPR (__consecutive)
+ {
+ // Characters 'a'..'z' are consecutive
+ if (std::isalpha(__c) && (__lc - 'a') < __b)
+ __c = __lc - 'a' + 10;
Is that alright? I have my eyes set on the following line in particular:
+ unsigned char __lc = std::tolower(__c);
Correct me if I'm wrong but that's locale sensitive. As it turns out in
a Turkish locale the lowercase for is <ı> "U+0131 LATIN SMALL LETTER
DOTLESS I".
I installed a Turkish locale on an old system of mine (Linux, UTF-8) and
sketched out the lines to quickly test the logic. As best as I can tell
the following happens when the global locale is set to Turkish:
- __lc is 'I' unchanged because <ı> U+0131 would encode to two UTF-8
codepoints aka can't fit in a char
- the test succeeds
- the computed value that ends up being written to __c is decimal -14
Keep in mind I didn't run the actual code from the patch, so I may have
got something wrong. Still, the Turkish i/İ and ı/I pairs are just one
example in one locale. Are std::tolower and std::isalpha the right tools
for this job? (These are the only two locale sensitive parts I
noticed in the code.)
