[PATCH] D108003: [Clang] Extend -Wbool-operation to warn about bitwise and of bools with side effects

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

I suggest you take all the techniques at 
http://graphics.stanford.edu/~seander/bithacks.html and make sure they don't 
cause a warning.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108003/new/

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80743: (PR46111) Desugar Elaborated types in Deduction Guides.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

I've got nothing to say here.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80743/new/

https://reviews.llvm.org/D80743



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69292: Proposal to add -Wtautological-compare to -Wall

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

Sorry I'm late to the party; I've been traveling for 3+ weeks.
I would like to be reassured that the following code will not warn:

  `
  long foo = ...; // some calculation
  if (foo < std::numeric_limits::min() || foo > 
std::numeric_limits::max()) .

This is important for systems where `sizeof(int) == sizeof(long)`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69292/new/

https://reviews.llvm.org/D69292



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69520: [libc++] Disallow dynamic -> static span conversions

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added inline comments.



Comment at: libcxx/test/std/containers/views/span.cons/span.fail.cpp:78
-
-//  Try to remove const and/or volatile (static -> static)
-{

Ok. The comment here is wrong; this is testing dynamic -> static.

However, why are you removing these (failing) tests?




Comment at: libcxx/test/std/containers/views/span.cons/span.pass.cpp:65
 }
-
-//  dynamic -> static

These cases (on the other hand) should be moved to a failure test.



Comment at: libcxx/test/std/containers/views/span.cons/span.pass.cpp:80
+
+  return s1.data() == nullptr && s1.size() == 0 && s2.data() == nullptr &&
+ s2.size() == 0 && s3.data() == nullptr && s3.size() == 0;

Please line these up like the other ones were. Makes it easy to see copy-pasta 
errors:
```
return s1.data() == nullptr && s1.size() == 0 
&& s2.data() == nullptr && s2.size() == 0
&& s3.data() == nullptr && s3.size() == 0;
```



Comment at: libcxx/test/std/containers/views/span.cons/span.pass.cpp:112
+std::span s3(s0d);// static -> dynamic
+return s1.data() == nullptr && s1.size() == 0 && s2.data() == nullptr &&
+   s2.size() == 0 && s3.data() == nullptr && s3.size() == 0;

same comment as above re: alignment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69520/new/

https://reviews.llvm.org/D69520



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68364: Prototype implementation of P0784R7: mark all members of std::allocator and std::allocator_traits as constexpr.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

Thanks for doing this, Richard.
A few things:

- Needs tests (as you said)
- The config macro stuff is all wrong (we build that file from a template)
- Use `__libcpp_is_constant_evaluated` instead of 
`__builtin_is_constant_evaluated` because it doesn't have to be `#ifdef`ed. 
(see r364873 and r364884)
- If we're in a consteval block, we shouldn't be testing for exceptions being 
disabled, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68364/new/

https://reviews.llvm.org/D68364



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67052: Add reference type transformation builtins

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

If you're going to do `__add__lvalue_reference`, `__add_rvalue_reference`, and 
`__remove_reference`, why not go all the way and add `__is_reference`, 
`__is_lvalue_reference` and `__is_rvalue_reference`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67052/new/

https://reviews.llvm.org/D67052



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40259: [libcxx] LWG2993: reference_wrapper conversion from T&

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

Did this ever get landed? If not, was there a reason?
Note that the synopsis at the top of `` also needs to be updated.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40259/new/

https://reviews.llvm.org/D40259



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37182: [libcxx] Special visibility macros for the experimental library

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

This is an old patch; is this still needed/desired?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D37182/new/

https://reviews.llvm.org/D37182



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D24372: [libcxx] Sprinkle constexpr over compressed_pair

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Is this patch relevant any more?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D24372/new/

https://reviews.llvm.org/D24372



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61366: [libcxx] [test] Don't assert that moved-from containers with non-POCMA allocators are empty.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

Is there a reason this hasn't been committed?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61366/new/

https://reviews.llvm.org/D61366



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43159: Modernize: Use nullptr more.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added inline comments.



Comment at: include/__threading_support:323
 bool __libcpp_thread_isnull(const __libcpp_thread_t *__t) {
-  return *__t == 0;
+  return *__t == nullptr;
 }

mclow.lists wrote:
> This one is wrong.
`__libcpp_thread_t` is an alias for an operating-system specific type.
On Mac OS, it is a pointer to some Darwin-specific type.
On Ubuntu, it is a `const unsigned long`.

You can't compare it to `nullptr`.



Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D43159/new/

https://reviews.llvm.org/D43159



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43159: Modernize: Use nullptr more.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added inline comments.



Comment at: include/algorithm:4431
 value_type* __p = __buff;
-for (_BidirectionalIterator __i = __first; __i != __middle; 
__d.__incr((value_type*)0), (void) ++__i, ++__p)
+for (_BidirectionalIterator __i = __first; __i != __middle; 
__d.__incr((value_type*)nullptr), (void) ++__i, ++__p)
 ::new(__p) value_type(_VSTD::move(*__i));

I'm not really a fan of casting `nullptr` to a `value_type *`. This change 
doesn't seem to add anything to the code; it's just longer. 

The call to `__incr` doesn't use the pointer at all. Maybe a better approach is 
to change 
```
template 
_LIBCPP_INLINE_VISIBILITY void __incr(_Tp*) _NOEXCEPT
{__incr(integral_constant::value>());}
```

to:
```
template 
_LIBCPP_INLINE_VISIBILITY void __incr() _NOEXCEPT
{__incr(integral_constant::value>());}
```

and remove the `nullptr`s from the calls in ``
Alternately, add an overload of `__incr` that takes a `nullptr_t`.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D43159/new/

https://reviews.llvm.org/D43159



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43159: Modernize: Use nullptr more.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In ``, you missed a couple of `(value_type*)0`. Line 3356, 3369, 
3486 and 3499.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D43159/new/

https://reviews.llvm.org/D43159



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43159: Modernize: Use nullptr more.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists requested changes to this revision.
mclow.lists added a comment.
This revision now requires changes to proceed.

Did you try to build libc++ or run the tests before submitting this?




Comment at: include/__threading_support:323
 bool __libcpp_thread_isnull(const __libcpp_thread_t *__t) {
-  return *__t == 0;
+  return *__t == nullptr;
 }

This one is wrong.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D43159/new/

https://reviews.llvm.org/D43159



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists closed this revision.
mclow.lists added a comment.

Committed as revision 364862


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked 2 inline comments as done.
mclow.lists added inline comments.



Comment at: libcxx/include/bit:203
+template
+inline _LIBCPP_INLINE_VISIBILITY constexpr
+enable_if_t<__bitop_unsigned_integer<_Tp>::value, _Tp>

EricWF wrote:
> Why the explicit inline hint?
Dang. Apparently I missed one. Will fix before committing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked 2 inline comments as done.
mclow.lists added inline comments.



Comment at: libcxx/include/bit:208
+const unsigned int __dig = numeric_limits<_Tp>::digits;
+return (__cnt % __dig) == 0
+? __t

EricWF wrote:
> Why did you choose to use the ternary operator here, but you use an `if` in 
> `rotr`?
Because Arthur suggested it, and said "Meh".


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists updated this revision to Diff 207395.
mclow.lists marked an inline comment as done.
mclow.lists added a comment.

Removed explicit `inline`s, changed ternary into `if`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262

Files:
  libcxx/include/bit
  libcxx/test/std/numerics/bit/bit.pow.two/ceil2.fail.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/ceil2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/floor2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/ispow2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/log2p1.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_one.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_zero.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_one.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_zero.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/popcount.pass.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotl.pass.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
  libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp

Index: libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp
@@ -0,0 +1,12 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+int main()
+{
+}
Index: libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
@@ -0,0 +1,181 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+// UNSUPPORTED: c++98, c++03, c++11, c++14, c++17 
+
+// template 
+//   constexpr int rotr(T x, unsigned int s) noexcept;
+
+// Remarks: This function shall not participate in overload resolution unless 
+//  T is an unsigned integer type
+
+#include 
+#include 
+#include 
+#include 
+
+#include "test_macros.h"
+
+class A{};
+enum   E1 : unsigned char { rEd };
+enum class E2 : unsigned char { red };
+
+template 
+constexpr bool constexpr_test()
+{
+const T max = std::numeric_limits::max();
+
+return std::rotr(T(128), 0) == T(128)
+   &&  std::rotr(T(128), 1) == T( 64)
+   &&  std::rotr(T(128), 2) == T( 32)
+   &&  std::rotr(T(128), 3) == T( 16)
+   &&  std::rotr(T(128), 4) == T(  8)
+   &&  std::rotr(T(128), 5) == T(  4)
+   &&  std::rotr(T(128), 6) == T(  2)
+   &&  std::rotr(T(128), 7) == T(  1)
+   &&  std::rotr(max, 0)  == max
+   &&  std::rotr(max, 1)  == max
+   &&  std::rotr(max, 2)  == max
+   &&  std::rotr(max, 3)  == max
+   &&  std::rotr(max, 4)  == max
+   &&  std::rotr(max, 5)  == max
+   &&  std::rotr(max, 6)  == max
+   &&  std::rotr(max, 7)  == max
+  ;
+}
+
+
+template 
+void runtime_test()
+{
+ASSERT_SAME_TYPE(T, decltype(std::rotr(T(0), 0)));
+ASSERT_NOEXCEPT( std::rotr(T(0), 0));
+const T max = std::numeric_limits::max();
+const T val = std::numeric_limits::max() - 1;
+
+const T uppers [] = {
+max,  // not used
+max - max,// 000 .. 0
+max - (max >> 1), // 800 .. 0
+max - (max >> 2), // C00 .. 0
+max - (max >> 3), // E00 .. 0
+max - (max >> 4), // F00 .. 0
+max - (max >> 5), // F80 .. 0
+max - (max >> 6), // FC0 .. 0
+max - (max >> 7), // FE0 .. 0
+};
+
+assert( std::rotr(val, 0) == val);
+assert( std::rotr(val, 1) == T((val >> 1) +  uppers[1]));
+assert( std::rotr(val, 2) == T((val >> 2) +  uppers[2]));
+assert( std::rotr(val, 3) == T((val >> 3) +  uppers[3]));
+assert( std::rotr(val, 4) == T((val >> 4) +  uppers[4]));
+assert( std::rotr(val, 5) == T((val >> 5) +  uppers[5]));
+assert( std::rotr(val, 6) == T((val >> 6) +  uppers[6]));
+assert( std::rotr(val, 7) == T((val >> 7) +  uppers[7]));
+}
+
+int main()
+{
+
+{
+auto lambda = [](auto x) -> decltype(std::rotr(x, 1U)) {};
+using L = decltype(lambda);
+
+static_assert( std::is_invocable_v, "");
+static_assert( std::is_invocable_v, "");
+static_assert( std::is_invocable_v, "");
+static_assert( std::is_invocable_v, "");
+
+static_assert( std::is_invocable_v, "");
+static_assert( std::is_invocable_v, "");
+static_assert( 

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked 5 inline comments as done.
mclow.lists added a comment.

I missed a couple of Eric's comments.




Comment at: libcxx/include/bit:211
+? __t
+: (__t << (__cnt % __dig)) | (__t >> (__dig - (__cnt % __dig)));
+}

xbolva00 wrote:
> mclow.lists wrote:
> > Quuxplusone wrote:
> > > No sane compiler would actually generate three `mod` operations for the 
> > > three instances of repeated subexpression `__cnt % __dig`, would they?
> > At `-O0`? Sure it would.
> Hoist it? 
> Hoist it?

IMHO, people building at `-O0` are not people who care about code 
size/performance.




Comment at: libcxx/include/bit:384
+log2p1(_Tp __t) noexcept
+{ return __t == 0 ? 0 : __bit_log2(__t) + 1; }
+

Quuxplusone wrote:
> mclow.lists wrote:
> > EricWF wrote:
> > > `__bit_log2` needs to be qualified to prevent ADL.
> > See above. ADL on what? There are no user-defined types here.
> FWIW, my intuition still agrees with Eric's: adding `_VSTD::` throughout 
> might not help anything, but if it wouldn't hurt, //and// if it stops every 
> future code-reviewer from making the same exact comment about ADL, shouldn't 
> you just do it? Wouldn't it save everyone some time?
I disagree. When I review a chunk of code, if I see `_VSTD:`, I think "why is 
this here?", and I go looking for ADL points. I think it //slows down// 
reviewing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists updated this revision to Diff 207394.
mclow.lists marked an inline comment as done.
mclow.lists added a comment.

Updated patch based on Eric's comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262

Files:
  libcxx/include/bit
  libcxx/test/std/numerics/bit/bit.pow.two/ceil2.fail.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/ceil2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/floor2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/ispow2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/log2p1.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_one.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_zero.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_one.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_zero.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/popcount.pass.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotl.pass.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
  libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp

Index: libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp
@@ -0,0 +1,12 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+int main()
+{
+}
Index: libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
@@ -0,0 +1,181 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+// UNSUPPORTED: c++98, c++03, c++11, c++14, c++17 
+
+// template 
+//   constexpr int rotr(T x, unsigned int s) noexcept;
+
+// Remarks: This function shall not participate in overload resolution unless 
+//  T is an unsigned integer type
+
+#include 
+#include 
+#include 
+#include 
+
+#include "test_macros.h"
+
+class A{};
+enum   E1 : unsigned char { rEd };
+enum class E2 : unsigned char { red };
+
+template 
+constexpr bool constexpr_test()
+{
+const T max = std::numeric_limits::max();
+
+return std::rotr(T(128), 0) == T(128)
+   &&  std::rotr(T(128), 1) == T( 64)
+   &&  std::rotr(T(128), 2) == T( 32)
+   &&  std::rotr(T(128), 3) == T( 16)
+   &&  std::rotr(T(128), 4) == T(  8)
+   &&  std::rotr(T(128), 5) == T(  4)
+   &&  std::rotr(T(128), 6) == T(  2)
+   &&  std::rotr(T(128), 7) == T(  1)
+   &&  std::rotr(max, 0)  == max
+   &&  std::rotr(max, 1)  == max
+   &&  std::rotr(max, 2)  == max
+   &&  std::rotr(max, 3)  == max
+   &&  std::rotr(max, 4)  == max
+   &&  std::rotr(max, 5)  == max
+   &&  std::rotr(max, 6)  == max
+   &&  std::rotr(max, 7)  == max
+  ;
+}
+
+
+template 
+void runtime_test()
+{
+ASSERT_SAME_TYPE(T, decltype(std::rotr(T(0), 0)));
+ASSERT_NOEXCEPT( std::rotr(T(0), 0));
+const T max = std::numeric_limits::max();
+const T val = std::numeric_limits::max() - 1;
+
+const T uppers [] = {
+max,  // not used
+max - max,// 000 .. 0
+max - (max >> 1), // 800 .. 0
+max - (max >> 2), // C00 .. 0
+max - (max >> 3), // E00 .. 0
+max - (max >> 4), // F00 .. 0
+max - (max >> 5), // F80 .. 0
+max - (max >> 6), // FC0 .. 0
+max - (max >> 7), // FE0 .. 0
+};
+
+assert( std::rotr(val, 0) == val);
+assert( std::rotr(val, 1) == T((val >> 1) +  uppers[1]));
+assert( std::rotr(val, 2) == T((val >> 2) +  uppers[2]));
+assert( std::rotr(val, 3) == T((val >> 3) +  uppers[3]));
+assert( std::rotr(val, 4) == T((val >> 4) +  uppers[4]));
+assert( std::rotr(val, 5) == T((val >> 5) +  uppers[5]));
+assert( std::rotr(val, 6) == T((val >> 6) +  uppers[6]));
+assert( std::rotr(val, 7) == T((val >> 7) +  uppers[7]));
+}
+
+int main()
+{
+
+{
+auto lambda = [](auto x) -> decltype(std::rotr(x, 1U)) {};
+using L = decltype(lambda);
+
+static_assert( std::is_invocable_v, "");
+static_assert( std::is_invocable_v, "");
+static_assert( std::is_invocable_v, "");
+static_assert( std::is_invocable_v, "");
+
+static_assert( std::is_invocable_v, "");
+static_assert( std::is_invocable_v, "");
+static_assert( 

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked 7 inline comments as done.
mclow.lists added inline comments.



Comment at: libcxx/include/bit:236
+
+if  constexpr (sizeof(_Tp) <= sizeof(unsigned int))
+return __clz(static_cast(__t))

EricWF wrote:
> Weird indentation. Clang-format please. 
The indentation is purposeful. It illustrates the structure of the code.



Comment at: libcxx/include/bit:240
+else if constexpr (sizeof(_Tp) <= sizeof(unsigned long))
+return __clz(static_cast(__t))
+  - (numeric_limits::digits - 
numeric_limits<_Tp>::digits);

EricWF wrote:
> Do we even need `if constexpr` here?
Yes. When instantiated with `_Tp == unsigned long` (say), this should be a 
single call to `__clz`, w/o any other code.



Comment at: libcxx/include/bit:251
+while (true) {
+__t = rotr(__t, __ulldigits);
+if ((__iter = countl_zero(static_cast(__t))) 
!= __ulldigits)

EricWF wrote:
> This call needs to be qualified.
I don't see why. `_Tp` must be an unsigned integral type.



Comment at: libcxx/include/bit:322
+if  constexpr (sizeof(_Tp) <= sizeof(unsigned int))
+return __popcount(static_cast(__t));
+else if constexpr (sizeof(_Tp) <= sizeof(unsigned long))

EricWF wrote:
> Do we need `if constexpr` here.
Yes. See above.



Comment at: libcxx/include/bit:366
+#ifndef _LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED
+if (!__builtin_is_constant_evaluated ())
+_LIBCPP_DEBUG_ASSERT( __n != numeric_limits<_Tp>::digits, "Bad input 
to ceil2" );

EricWF wrote:
> You don't need to guard debug assertions in C++14 constexpr functions.
Good to know; thanks.



Comment at: libcxx/include/bit:384
+log2p1(_Tp __t) noexcept
+{ return __t == 0 ? 0 : __bit_log2(__t) + 1; }
+

EricWF wrote:
> `__bit_log2` needs to be qualified to prevent ADL.
See above. ADL on what? There are no user-defined types here.



Comment at: libcxx/test/std/numerics/bit/bitops.count/countr_zero.fail.cpp:29
+{
+   (void) std::countr_zero(static_cast(2)); // expected-error 
{{no matching function for call to 'countr_zero'}}  
+   (void) std::countr_zero(static_cast(2));  // expected-error 
{{no matching function for call to 'countr_zero'}}  

EricWF wrote:
> Use `is_invocable`.
I can do that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62782: Fix -Wdouble-promotion warnings.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.
This revision is now accepted and ready to land.

Sorry for the slow response.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62782/new/

https://reviews.llvm.org/D62782



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists updated this revision to Diff 206571.
mclow.lists marked 4 inline comments as done.
mclow.lists added a comment.

Address a couple of review comments. Added a few more `uint128_t` rotation tests


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262

Files:
  libcxx/include/bit
  libcxx/test/std/numerics/bit/bit.pow.two/ceil2.fail.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/ceil2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/floor2.fail.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/floor2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/ispow2.fail.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/ispow2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/log2p1.fail.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/log2p1.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_one.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_one.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_zero.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_zero.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_one.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_one.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_zero.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_zero.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/popcount.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/popcount.pass.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotl.fail.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotl.pass.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotr.fail.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
  libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp

Index: libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp
@@ -0,0 +1,12 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+int main()
+{
+}
Index: libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
@@ -0,0 +1,131 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+// UNSUPPORTED: c++98, c++03, c++11, c++14, c++17 
+
+// template 
+//   constexpr int rotr(T x, unsigned int s) noexcept;
+
+// Remarks: This function shall not participate in overload resolution unless 
+//  T is an unsigned integer type
+
+#include 
+#include 
+#include 
+
+#include "test_macros.h"
+
+template 
+constexpr bool constexpr_test()
+{
+const T max = std::numeric_limits::max();
+
+return std::rotr(T(128), 0) == T(128)
+   &&  std::rotr(T(128), 1) == T( 64)
+   &&  std::rotr(T(128), 2) == T( 32)
+   &&  std::rotr(T(128), 3) == T( 16)
+   &&  std::rotr(T(128), 4) == T(  8)
+   &&  std::rotr(T(128), 5) == T(  4)
+   &&  std::rotr(T(128), 6) == T(  2)
+   &&  std::rotr(T(128), 7) == T(  1)
+   &&  std::rotr(max, 0)  == max
+   &&  std::rotr(max, 1)  == max
+   &&  std::rotr(max, 2)  == max
+   &&  std::rotr(max, 3)  == max
+   &&  std::rotr(max, 4)  == max
+   &&  std::rotr(max, 5)  == max
+   &&  std::rotr(max, 6)  == max
+   &&  std::rotr(max, 7)  == max
+  ;
+}
+
+
+template 
+void runtime_test()
+{
+ASSERT_SAME_TYPE(T, decltype(std::rotr(T(0), 0)));
+ASSERT_NOEXCEPT( std::rotr(T(0), 0));
+const T max = std::numeric_limits::max();
+const T val = std::numeric_limits::max() - 1;
+
+const T uppers [] = {
+max,  // not used
+max - max,// 000 .. 0
+max - (max >> 1), // 800 .. 0
+max - (max >> 2), // C00 .. 0
+max - (max >> 3), // E00 .. 0
+max - (max >> 4), // F00 .. 0
+max - (max >> 5), // F80 .. 0
+max - (max >> 6), // FC0 .. 0
+max - (max >> 7), // FE0 .. 0
+};
+
+assert( std::rotr(val, 0) == val);
+assert( std::rotr(val, 1) == T((val >> 1) +  uppers[1]));
+assert( std::rotr(val, 2) == T((val >> 2) +  uppers[2]));
+assert( std::rotr(val, 3) == T((val >> 3) +  uppers[3]));
+assert( std::rotr(val, 4) == T((val >> 4) +  uppers[4]));
+assert( std::rotr(val, 5) == T((val >> 5) +  uppers[5]));
+assert( std::rotr(val, 

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In D51262#1557154 , @jwakely wrote:

> In D51262#1213514 , @mclow.lists 
> wrote:
>
> > I should also mention that as a conforming extension, I have implemented 
> > the non-numeric bit operations for `std::byte`
>
>
> I thought there was a DR or proposal to enable that, but I don't see one now.


There was, but LEWG rejected it, so I took that out of this patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked an inline comment as done.
mclow.lists added inline comments.



Comment at: libcxx/include/bit:378
+   const unsigned __retVal = 1u << (__n + __extra);
+   return (_Tp) (__retVal >> __extra);
+}

mclow.lists wrote:
> mclow.lists wrote:
> > Quuxplusone wrote:
> > > mclow.lists wrote:
> > > > mclow.lists wrote:
> > > > > Quuxplusone wrote:
> > > > > > Why so complicated? Is there a unit test that demonstrates why you 
> > > > > > can't just use `return _Tp{1} << __n;` in this case as well?
> > > > > > 
> > > > > > Also, is this a kosher use of `sizeof(X) * 8` as a stand-in for 
> > > > > > `numeric_limits::digits`?
> > > > > > 
> > > > > > Also, speaking of unit tests, I don't see any unit tests for e.g. 
> > > > > > `std::ceil2(256)` or `std::ceil2(65536)`. Shouldn't there be some?
> > > > > Yes. I want to generate some UB here, so that this is not a "core 
> > > > > constant expression" as per P1355.
> > > > Yes, the `ceil2.fail.cpp` test will not fail (for short types) if I 
> > > > just return `_Tp{1} << __n;` - because of integer promotion.
> > > > 
> > > Yikes. Sounds like there's some "library maintainer"ship going on here, 
> > > then. Naively, I would have thought that the Working Draft had left the 
> > > behavior of such constructs undefined in order to make the library 
> > > vendor's life **easier** and the code **more efficient**. But you're 
> > > saying that you need to pessimize the code here in order to make it 
> > > "sufficiently undefined" so that its behavior at compile time will be 
> > > well-defined and produce a diagnostic instead of being undefined and 
> > > producing garbage?
> > > 
> > > Maybe WG21 needs to invent a "really actually undefined behavior" that 
> > > does not have unwanted interactions with constexpr, so that library 
> > > writers can go back to generating fast clean code!
> > > 
> > > Rant aside, why don't you just do `_LIBCPP_ASSERT(__n < 
> > > numeric_limits<_Tp>::digits);` and leave it at that?  An assertion 
> > > failure isn't a compile-time constant, is it?
> > > Also, is this a kosher use of sizeof(X) * 8 as a stand-in for 
> > > numeric_limits::digits?
> > 
> > Good catch.
> that's exactly what P1355 says.
There are three things that I'd like this code to do in the case of bad inputs:
* Produce a diagnostic if called at constexpr time (required by P1355)
* Cause UBSAN to catch it at runtime
* Cause a libc++ assertion to go off if they are enabled.

Note that these are all more or less independent.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked an inline comment as done.
mclow.lists added inline comments.



Comment at: libcxx/include/bit:378
+   const unsigned __retVal = 1u << (__n + __extra);
+   return (_Tp) (__retVal >> __extra);
+}

mclow.lists wrote:
> Quuxplusone wrote:
> > mclow.lists wrote:
> > > mclow.lists wrote:
> > > > Quuxplusone wrote:
> > > > > Why so complicated? Is there a unit test that demonstrates why you 
> > > > > can't just use `return _Tp{1} << __n;` in this case as well?
> > > > > 
> > > > > Also, is this a kosher use of `sizeof(X) * 8` as a stand-in for 
> > > > > `numeric_limits::digits`?
> > > > > 
> > > > > Also, speaking of unit tests, I don't see any unit tests for e.g. 
> > > > > `std::ceil2(256)` or `std::ceil2(65536)`. Shouldn't there be some?
> > > > Yes. I want to generate some UB here, so that this is not a "core 
> > > > constant expression" as per P1355.
> > > Yes, the `ceil2.fail.cpp` test will not fail (for short types) if I just 
> > > return `_Tp{1} << __n;` - because of integer promotion.
> > > 
> > Yikes. Sounds like there's some "library maintainer"ship going on here, 
> > then. Naively, I would have thought that the Working Draft had left the 
> > behavior of such constructs undefined in order to make the library vendor's 
> > life **easier** and the code **more efficient**. But you're saying that you 
> > need to pessimize the code here in order to make it "sufficiently 
> > undefined" so that its behavior at compile time will be well-defined and 
> > produce a diagnostic instead of being undefined and producing garbage?
> > 
> > Maybe WG21 needs to invent a "really actually undefined behavior" that does 
> > not have unwanted interactions with constexpr, so that library writers can 
> > go back to generating fast clean code!
> > 
> > Rant aside, why don't you just do `_LIBCPP_ASSERT(__n < 
> > numeric_limits<_Tp>::digits);` and leave it at that?  An assertion failure 
> > isn't a compile-time constant, is it?
> > Also, is this a kosher use of sizeof(X) * 8 as a stand-in for 
> > numeric_limits::digits?
> 
> Good catch.
that's exactly what P1355 says.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked 4 inline comments as done.
mclow.lists added inline comments.



Comment at: libcxx/include/bit:252
+while (true) {
+__t = rotr<_Tp>(__t, __ulldigits);
+if ((__iter = countl_zero(static_cast(__t))) 
!= __ulldigits)

Quuxplusone wrote:
> Since `__t` is already of type `_Tp`, the explicit template argument `<_Tp>` 
> is unnecessary here.
> Also, is the ADL here intentional?
Right about the `<_Tp>`



Comment at: libcxx/include/bit:253
+__t = rotr<_Tp>(__t, __ulldigits);
+if ((__iter = countl_zero(static_cast(__t))) 
!= __ulldigits)
+break;

Quuxplusone wrote:
> I forget the rules of name hiding; is this `countl_zero` also an ADL call? 
> (My experimentation implies it's not? but it would still be clearer to use 
> `_VSTD::countl_zero` if that's what you mean.)
> 
> As a general rule, I strongly recommend writing side-effects //outside// of 
> `if` conditions; like, on the previous line or something; unless writing it 
> all on a single line has some concrete benefit.
Not an ADL call, because `_Tp` is always a built-in type.



Comment at: libcxx/include/bit:365
+if (__t < 2) return 1;
+const unsigned __n = numeric_limits<_Tp>::digits - countl_zero((_Tp)(__t - 
1u));
+

Quuxplusone wrote:
> Incidentally, why `(_Tp)(__t - 1u)` instead of the formula `__t - _Tp{1}` 
> used elsewhere in this header?
Because I want to end up with a value of type `_Tp`, and (for short types), 
`__t - _Tp{1}` doesn't get me that. (Integer promotion).  `((char) 23) - 
((char) 1)` is not type `char`



Comment at: libcxx/include/bit:378
+   const unsigned __retVal = 1u << (__n + __extra);
+   return (_Tp) (__retVal >> __extra);
+}

Quuxplusone wrote:
> mclow.lists wrote:
> > mclow.lists wrote:
> > > Quuxplusone wrote:
> > > > Why so complicated? Is there a unit test that demonstrates why you 
> > > > can't just use `return _Tp{1} << __n;` in this case as well?
> > > > 
> > > > Also, is this a kosher use of `sizeof(X) * 8` as a stand-in for 
> > > > `numeric_limits::digits`?
> > > > 
> > > > Also, speaking of unit tests, I don't see any unit tests for e.g. 
> > > > `std::ceil2(256)` or `std::ceil2(65536)`. Shouldn't there be some?
> > > Yes. I want to generate some UB here, so that this is not a "core 
> > > constant expression" as per P1355.
> > Yes, the `ceil2.fail.cpp` test will not fail (for short types) if I just 
> > return `_Tp{1} << __n;` - because of integer promotion.
> > 
> Yikes. Sounds like there's some "library maintainer"ship going on here, then. 
> Naively, I would have thought that the Working Draft had left the behavior of 
> such constructs undefined in order to make the library vendor's life 
> **easier** and the code **more efficient**. But you're saying that you need 
> to pessimize the code here in order to make it "sufficiently undefined" so 
> that its behavior at compile time will be well-defined and produce a 
> diagnostic instead of being undefined and producing garbage?
> 
> Maybe WG21 needs to invent a "really actually undefined behavior" that does 
> not have unwanted interactions with constexpr, so that library writers can go 
> back to generating fast clean code!
> 
> Rant aside, why don't you just do `_LIBCPP_ASSERT(__n < 
> numeric_limits<_Tp>::digits);` and leave it at that?  An assertion failure 
> isn't a compile-time constant, is it?
> Also, is this a kosher use of sizeof(X) * 8 as a stand-in for 
> numeric_limits::digits?

Good catch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked an inline comment as done.
mclow.lists added inline comments.



Comment at: libcxx/include/bit:378
+   const unsigned __retVal = 1u << (__n + __extra);
+   return (_Tp) (__retVal >> __extra);
+}

mclow.lists wrote:
> Quuxplusone wrote:
> > Why so complicated? Is there a unit test that demonstrates why you can't 
> > just use `return _Tp{1} << __n;` in this case as well?
> > 
> > Also, is this a kosher use of `sizeof(X) * 8` as a stand-in for 
> > `numeric_limits::digits`?
> > 
> > Also, speaking of unit tests, I don't see any unit tests for e.g. 
> > `std::ceil2(256)` or `std::ceil2(65536)`. Shouldn't there be some?
> Yes. I want to generate some UB here, so that this is not a "core constant 
> expression" as per P1355.
Yes, the `ceil2.fail.cpp` test will not fail (for short types) if I just return 
`_Tp{1} << __n;` - because of integer promotion.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked 2 inline comments as done.
mclow.lists added inline comments.



Comment at: libcxx/include/bit:199
+!is_same_v, char32_t>
+ > {};
+

Quuxplusone wrote:
> Given how heavily the code controlled by this trait depends on 
> `numeric_limits<_Tp>`, would it make sense to add something in here about how 
> that specialization should exist?
> 
> I agree with @EricWF's earlier comment: I can't think of any types that would 
> satisfy `(!is_integral_v<_Tp>) && is_unsigned_v<_Tp>`.  (But just TIL that 
> the signedness of `wchar_t` is implementation-defined.)
 `is_unsigned_v` could be true, but `is_integral_v` is not ever 
true.



Comment at: libcxx/include/bit:211
+? __t
+: (__t << (__cnt % __dig)) | (__t >> (__dig - (__cnt % __dig)));
+}

Quuxplusone wrote:
> No sane compiler would actually generate three `mod` operations for the three 
> instances of repeated subexpression `__cnt % __dig`, would they?
At `-O0`? Sure it would.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked an inline comment as done.
mclow.lists added inline comments.



Comment at: libcxx/include/bit:378
+   const unsigned __retVal = 1u << (__n + __extra);
+   return (_Tp) (__retVal >> __extra);
+}

Quuxplusone wrote:
> Why so complicated? Is there a unit test that demonstrates why you can't just 
> use `return _Tp{1} << __n;` in this case as well?
> 
> Also, is this a kosher use of `sizeof(X) * 8` as a stand-in for 
> `numeric_limits::digits`?
> 
> Also, speaking of unit tests, I don't see any unit tests for e.g. 
> `std::ceil2(256)` or `std::ceil2(65536)`. Shouldn't there be some?
Yes. I want to generate some UB here, so that this is not a "core constant 
expression" as per P1355.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

> Also are the unsigned types that aren't integral?

I believe that people are allowed to specialize `is_unsigned` for their own 
(bignum, say) types.
However, `is_integral` is not extensible. It refers to the types in 
`[basic.fundamental]`

> Type `bool` is a distinct type that has the same object representation, value 
> representation, and alignment requirements as an implementation-defined 
> unsigned integer type.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked 4 inline comments as done.
mclow.lists added inline comments.



Comment at: include/bit:254
+
+if  constexpr (sizeof(_Tp) <= sizeof(unsigned int))
+   return __clz(static_cast(__t))

EricWF wrote:
> Cool use of `if constexpr`.
> 
> Please clang-format these changes.
I really like this formatting; it reflects the structure of the code.



Comment at: include/bit:405
+
+template 
+inline _LIBCPP_INLINE_VISIBILITY constexpr

EricWF wrote:
> Please write the SFINAE using a default template parameter.
> 
> See 
> http://libcxx.llvm.org/docs/DesignDocs/ExtendedCXX03Support.html#use-default-template-parameters-for-sfinae
I think that document is misguided in cases like this; the `enable_if` on the 
return type cannot be "interfered with" by the caller the way an extra template 
parameter can be.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists updated this revision to Diff 206339.
mclow.lists added a comment.

Update this patch to implement P1355R2  which makes 
out-of-bound inputs for `ceil2`UB. This was easy for integer types that are at 
least as big as `int`, but harder for smaller ones.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51262/new/

https://reviews.llvm.org/D51262

Files:
  libcxx/include/bit
  libcxx/test/std/numerics/bit/bit.pow.two/ceil2.fail.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/ceil2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/floor2.fail.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/floor2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/ispow2.fail.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/ispow2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/log2p1.fail.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/log2p1.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_one.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_one.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_zero.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_zero.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_one.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_one.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_zero.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_zero.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/popcount.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/popcount.pass.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotl.fail.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotl.pass.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotr.fail.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
  libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp

Index: libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp
@@ -0,0 +1,12 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+int main()
+{
+}
Index: libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
@@ -0,0 +1,118 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+// UNSUPPORTED: c++98, c++03, c++11, c++14, c++17 
+
+// template 
+//   constexpr int rotr(T x, unsigned int s) noexcept;
+
+// Remarks: This function shall not participate in overload resolution unless 
+//	T is an unsigned integer type
+
+#include 
+#include 
+#include 
+
+#include "test_macros.h"
+
+template 
+constexpr bool constexpr_test()
+{
+	const T max = std::numeric_limits::max();
+
+	return std::rotr(T(128), 0) == T(128)
+	   &&  std::rotr(T(128), 1) == T( 64)
+	   &&  std::rotr(T(128), 2) == T( 32)
+	   &&  std::rotr(T(128), 3) == T( 16)
+	   &&  std::rotr(T(128), 4) == T(  8)
+	   &&  std::rotr(T(128), 5) == T(  4)
+	   &&  std::rotr(T(128), 6) == T(  2)
+	   &&  std::rotr(T(128), 7) == T(  1)
+	   &&  std::rotr(max, 0)  == max
+	   &&  std::rotr(max, 1)  == max
+	   &&  std::rotr(max, 2)  == max
+	   &&  std::rotr(max, 3)  == max
+	   &&  std::rotr(max, 4)  == max
+	   &&  std::rotr(max, 5)  == max
+	   &&  std::rotr(max, 6)  == max
+	   &&  std::rotr(max, 7)  == max
+	  ;
+}
+
+
+template 
+void runtime_test()
+{
+	ASSERT_SAME_TYPE(T, decltype(std::rotr(T(0), 0)));
+	ASSERT_NOEXCEPT( std::rotr(T(0), 0));
+	const T max = std::numeric_limits::max();
+	const T val = std::numeric_limits::max() - 1;
+
+	const T uppers [] = {
+		max,	  // not used
+		max - max,	  // 000 .. 0
+		max - (max >> 1), // 800 .. 0
+		max - (max >> 2), // C00 .. 0
+		max - (max >> 3), // E00 .. 0
+		max - (max >> 4), // F00 .. 0
+		max - (max >> 5), // F80 .. 0
+		max - (max >> 6), // FC0 .. 0
+		max - (max >> 7), // FE0 .. 0
+		};
+	
+	assert( std::rotr(val, 0) == val);
+	assert( std::rotr(val, 1) == T((val >> 1) +  uppers[1]));
+	assert( std::rotr(val, 2) == T((val >> 2) +  uppers[2]));
+	assert( std::rotr(val, 3) == T((val >> 3) +  uppers[3]));
+	assert( std::rotr(val, 4) == T((val >> 4) +  uppers[4]));
+	assert( std::rotr(val, 5) == T((val >> 5) +  uppers[5]));
+	assert( std::rotr(val, 6) == T((val >> 6) +  uppers[6]));
+	assert( std::rotr(val, 7) == T((val 

[PATCH] D44865: [libc++] Implement P0608R3 - A sane variant converting constructor

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.
This revision is now accepted and ready to land.

This looks good to me. Thanks!


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44865/new/

https://reviews.llvm.org/D44865



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44865: [libc++] Implement P0608R3 - A sane variant converting constructor

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

The patch looks fine to me, but I think you should consider making a couple of 
`T.fail.cpp` tests, and check to make sure you get the "right error".




Comment at: 
test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp:130
   {
 using V = std::variant;
 static_assert(!std::is_assignable::value, "ambiguous");

If you really want to check that these are "ambiguous" , or "no matching 
operator=", etc, the way to do that is to define a fail.cpp test, and check the 
error messages.
 


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44865/new/

https://reviews.llvm.org/D44865



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44865: [libc++] Implement P0608R3 - A sane variant converting constructor

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In D44865#1051228 , @EricWF wrote:

> This LGTM.
>
> Also I would like @mclow.lists input about applying this DR early since LWG
>  hasn't commented on it yet.


Even though LWG didn't vote it out as a DR; that's what it is, and I think that 
we would be doing our users a disservice by not providing this in C++17.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44865/new/

https://reviews.llvm.org/D44865



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44865: [libc++] Implement P0608R3 - A sane variant converting constructor

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In D44865#1391996 , @lichray wrote:

> Ping @mclow.lists @EricWF  ; the patch still applies, is there any other 
> thing I need to address?


Just a note: P0608R3 was adopted in San Diego.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44865/new/

https://reviews.llvm.org/D44865



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43226: __threading_support: Remove (void) in favor of ().

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.
This revision is now accepted and ready to land.

This looks fine to me.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D43226/new/

https://reviews.llvm.org/D43226



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62782: Fix -Wdouble-promotion warnings.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

This looks fine to me; but where were you getting these warnings?


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62782/new/

https://reviews.llvm.org/D62782



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43159: Modernize: Use nullptr more.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In D43159#1526170 , @brucem wrote:

> Can we revive this review? I'd still like to land this ...


What was the result of testing with `-std=c++98` and/or `-std=gnu++98` ?
The code changes look fine; but as @Ericwf said


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D43159/new/

https://reviews.llvm.org/D43159



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62654: [Docs] Modernize references to macOS

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

I'm fine with the libc++ changes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62654/new/

https://reviews.llvm.org/D62654



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59253: [AIX][libcxx] AIX system headers need stdint.h and inttypes.h to be re-enterable when macro _STD_TYPES_T is defined

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added inline comments.



Comment at: libcxx/include/stdint.h:16
+#endif // _STD_TYPES_T
 
 /*

I don't think that this will do what you want it to.
Is this a supported use case?

```
#include 
#define _STD_TYPES_T
#include 
```




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59253/new/

https://reviews.llvm.org/D59253



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62034: compiler-rt/lib/builtins: s/#include /#include /g to match proper case.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

I have no objection to this; but I'm not really confident that this won't break 
more than it solves.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62034/new/

https://reviews.llvm.org/D62034



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61858: Make `__is_base_of` more friendly with unions

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists closed this revision.
mclow.lists added a comment.

Committed as revision 360614


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61858/new/

https://reviews.llvm.org/D61858



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61858: Make `__is_base_of` more friendly with unions

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In D61858#1500270 , @ldionne wrote:

> Shouldn't we also add unit tests for PR41843 in libc++?


Yes. But I want to do that later.
After this has landed (and probably wait a week for all the bots that are 
running trunk to update).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61858/new/

https://reviews.llvm.org/D61858



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61858: Make `__is_base_of` more friendly with unions

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

Note: There are tabs in `clang/test/SemaCXX/type-traits.cpp` that I didn't 
remove because it would have cluttered up the diff.
I can de-tabify the file when it is committed if people want.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61858/new/

https://reviews.llvm.org/D61858



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61858: Make `__is_base_of` more friendly with unions

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists created this revision.
mclow.lists added reviewers: rsmith, t.p.northover, EricWF, ldionne.
mclow.lists added a project: clang.
Herald added a subscriber: dexonsmith.

Unions are never base classes, and never have base classes.
It doesn't matter if they are complete or not. See http://llvm.org/PR41843


https://reviews.llvm.org/D61858

Files:
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/SemaCXX/type-traits.cpp


Index: clang/test/SemaCXX/type-traits.cpp
===
--- clang/test/SemaCXX/type-traits.cpp
+++ clang/test/SemaCXX/type-traits.cpp
@@ -14,6 +14,7 @@
 enum Enum { EV };
 struct POD { Enum e; int i; float f; NonPOD* p; };
 struct Empty {};
+struct IncompleteStruct;
 typedef Empty EmptyAr[10];
 typedef Empty EmptyArNB[];
 typedef Empty EmptyArMB[1][2];
@@ -1915,6 +1916,20 @@
   { int arr[F(__is_base_of(Base, NonderivedTemp))]; }
   { int arr[F(__is_base_of(Base, UndefinedTemp))]; } // expected-error 
{{implicit instantiation of undefined template 'UndefinedTemp'}}
 
+  { int arr[F(__is_base_of(IncompleteUnion, IncompleteUnion))]; }
+  { int arr[F(__is_base_of(Union, IncompleteUnion))]; }
+  { int arr[F(__is_base_of(IncompleteUnion, Union))]; }
+  { int arr[F(__is_base_of(IncompleteStruct, IncompleteUnion))]; }
+  { int arr[F(__is_base_of(IncompleteUnion, IncompleteStruct))]; }
+  { int arr[F(__is_base_of(Empty, IncompleteUnion))]; }
+  { int arr[F(__is_base_of(IncompleteUnion, Empty))]; }
+  { int arr[F(__is_base_of(int, IncompleteUnion))]; }
+  { int arr[F(__is_base_of(IncompleteUnion, int))]; }
+  { int arr[F(__is_base_of(Empty, Union))]; }
+  { int arr[F(__is_base_of(Union, Empty))]; }
+  { int arr[F(__is_base_of(int, Empty))]; }
+  { int arr[F(__is_base_of(Union, int))]; }
+
   isBaseOfT();
   isBaseOfF();
 
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -5118,8 +5118,15 @@
 assert(Self.Context.hasSameUnqualifiedType(LhsT, RhsT)
  == (lhsRecord == rhsRecord));
 
+// Unions are never base classes, and never have base classes.
+// It doesn't matter if they are complete or not. See PR#41843
+if (lhsRecord && lhsRecord->getDecl()->isUnion())
+  return false;
+if (rhsRecord && rhsRecord->getDecl()->isUnion())
+  return false;
+
 if (lhsRecord == rhsRecord)
-  return !lhsRecord->getDecl()->isUnion();
+  return true;
 
 // C++0x [meta.rel]p2:
 //   If Base and Derived are class types and are different types


Index: clang/test/SemaCXX/type-traits.cpp
===
--- clang/test/SemaCXX/type-traits.cpp
+++ clang/test/SemaCXX/type-traits.cpp
@@ -14,6 +14,7 @@
 enum Enum { EV };
 struct POD { Enum e; int i; float f; NonPOD* p; };
 struct Empty {};
+struct IncompleteStruct;
 typedef Empty EmptyAr[10];
 typedef Empty EmptyArNB[];
 typedef Empty EmptyArMB[1][2];
@@ -1915,6 +1916,20 @@
   { int arr[F(__is_base_of(Base, NonderivedTemp))]; }
   { int arr[F(__is_base_of(Base, UndefinedTemp))]; } // expected-error {{implicit instantiation of undefined template 'UndefinedTemp'}}
 
+  { int arr[F(__is_base_of(IncompleteUnion, IncompleteUnion))]; }
+  { int arr[F(__is_base_of(Union, IncompleteUnion))]; }
+  { int arr[F(__is_base_of(IncompleteUnion, Union))]; }
+  { int arr[F(__is_base_of(IncompleteStruct, IncompleteUnion))]; }
+  { int arr[F(__is_base_of(IncompleteUnion, IncompleteStruct))]; }
+  { int arr[F(__is_base_of(Empty, IncompleteUnion))]; }
+  { int arr[F(__is_base_of(IncompleteUnion, Empty))]; }
+  { int arr[F(__is_base_of(int, IncompleteUnion))]; }
+  { int arr[F(__is_base_of(IncompleteUnion, int))]; }
+  { int arr[F(__is_base_of(Empty, Union))]; }
+  { int arr[F(__is_base_of(Union, Empty))]; }
+  { int arr[F(__is_base_of(int, Empty))]; }
+  { int arr[F(__is_base_of(Union, int))]; }
+
   isBaseOfT();
   isBaseOfF();
 
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -5118,8 +5118,15 @@
 assert(Self.Context.hasSameUnqualifiedType(LhsT, RhsT)
  == (lhsRecord == rhsRecord));
 
+// Unions are never base classes, and never have base classes.
+// It doesn't matter if they are complete or not. See PR#41843
+if (lhsRecord && lhsRecord->getDecl()->isUnion())
+  return false;
+if (rhsRecord && rhsRecord->getDecl()->isUnion())
+  return false;
+
 if (lhsRecord == rhsRecord)
-  return !lhsRecord->getDecl()->isUnion();
+  return true;
 
 // C++0x [meta.rel]p2:
 //   If Base and Derived are class types and are different types
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47358: : Implement {un,}synchronized_pool_resource.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In D47358#1438929 , @Quuxplusone wrote:

> Rebased. Added `_NOEXCEPT` to `upstream_resource()` and `options()` (this is 
> OK per [res.on.exception.handling]/5).


That's fine, but then we should have a test for that.
We have the macro `LIBCPP_ASSERT_NOEXCEPT` specifically for this purpose 
(libc++ - specific noexcept markings).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D47358/new/

https://reviews.llvm.org/D47358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60417: [libunwind] Add support for ARMv7-M architecture which uses the Thumb 2 ISA (unified syntax)

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

I have no problem with this.


Repository:
  rUNW libunwind

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60417/new/

https://reviews.llvm.org/D60417



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49863: [istream] Fix error flags and exceptions propagated from input stream operations

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.

I'm OK with this.  I think, strictly speaking, that libc++ is following the 
standard, and everyone else is not - but what everyone else is doing makes 
sense.




Comment at: libcxx/include/istream:365
 __input_arithmetic(basic_istream<_CharT, _Traits>& __is, _Tp& __n) {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-try
+ios_base::iostate __state = ios_base::goodbit;
+typename basic_istream<_CharT, _Traits>::sentry __s(__is);

lichray wrote:
> I like `auto` just FYI :)
You may like `auto`, but C++03 does not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49863/new/

https://reviews.llvm.org/D49863



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60069: Declare std::tuple_element as struct instead of class

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.

I'm less enthusiastic about this change than the one for PR39871, because there 
we were being inconsistent with ourselves.
However, my lack of enthusiasm is no reason not to land this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60069/new/

https://reviews.llvm.org/D60069



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60069: Declare std::tuple_element as struct instead of class

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In D60069#1449937 , @jdoerrie wrote:

> Right, there shouldn't be any inheritance. Some of the `public:` access 
> specifications are now redundant, though.


And I think that you should get rid of them.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60069/new/

https://reviews.llvm.org/D60069



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60069: Declare std::tuple_element as struct instead of class

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In D60069#1449928 , @mclow.lists wrote:

> Did you check the places that inherit from  `tuple_element`? The 
> public/private bits change between class and struct.


Never mind. I was thinking of something else; I don't think that anything 
inherits from `tuple_element`


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60069/new/

https://reviews.llvm.org/D60069



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60069: Declare std::tuple_element as struct instead of class

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

Did you check the places that inherit from  `tuple_element`? The public/private 
bits change between class and struct.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60069/new/

https://reviews.llvm.org/D60069



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40181: [libcxx] Allow to set locale on Windows.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.
Herald added a project: LLVM.

I have a bug report - https://bugs.llvm.org/show_bug.cgi?id=41131 - that says 
that we can reduce the number of calls in __libcpp_locale_guard from 10 to 2 by 
calling:
`setlocale(LC_ALL, ...)` .  Was this considered when this patch was created?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40181/new/

https://reviews.llvm.org/D40181



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59253: [AIX][libcxx] AIX system headers need stdint.h and inttypes.h to be re-enterable when macro _STD_TYPES_T is defined

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists requested changes to this revision.
mclow.lists added a comment.
This revision now requires changes to proceed.

I see no tests here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59253/new/

https://reviews.llvm.org/D59253



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37994: Implement LWG2946: More ambiguity in `string` vs. `string_view`

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists abandoned this revision.
mclow.lists added a comment.
Herald added a subscriber: jdoerfert.

This appears to have been applied in a slightly different form. Certainly the 
functionality is there. Closing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D37994/new/

https://reviews.llvm.org/D37994



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44823: [libcxx] Improving std::vector and std::deque perfomance

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In D44823#1375590 , @mclow.lists wrote:

> I just tried this (on Compiler Explorer) using LLVM 7, and the code for my 
> original test in https://bugs.llvm.org/show_bug.cgi?id=35637 is now optimal.


Pilot error - it's still the same.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44823/new/

https://reviews.llvm.org/D44823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44823: [libcxx] Improving std::vector and std::deque perfomance

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.
Herald added a subscriber: libcxx-commits.

I just tried this (on Compiler Explorer) using LLVM 7, and the code for my 
original test in https://bugs.llvm.org/show_bug.cgi?id=35637 is now optimal.
Looking briefly at your test case, it seems to be fixed now too. Can you 
confirm or disprove, please?


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44823/new/

https://reviews.llvm.org/D44823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D14686: Protect against overloaded comma in random_shuffle and improve tests

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists closed this revision.
mclow.lists added a comment.
Herald added a subscriber: llvm-commits.

(Finally) committed this as revision 352087.
I cut out most of the random_shuffle_rand.pass.cpp test, because it relied on 
C++11 features, and didn't work for C++03.
If you want to re-submit the test as a separate patch, I promise to review it 
in a more timely manner.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D14686/new/

https://reviews.llvm.org/D14686



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D16541: [libc++] Renable test/std/re/re.alg/re.alg.match/awk.pass.cpp

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists closed this revision.
mclow.lists added a comment.
Herald added a reviewer: EricWF.

For some reason, this test was not marked `// XFAIL gnu-linux` like all the 
other tests that use the `LOCALE_cs_CZ_ISO8859_2` locale.
In revision 352006, I uncommented this entire test, and added the XFAIL line


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D16541/new/

https://reviews.llvm.org/D16541



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26110: Add a check for GCC to the _LIBCPP_EXPLICIT define

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists closed this revision.
mclow.lists added a comment.

Yes, this appears to have been landed. Closing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D26110/new/

https://reviews.llvm.org/D26110



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28248: Work around GCC PR37804

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists closed this revision.
mclow.lists added a comment.

I changed the `_LIBCPP_TYPE_VIS_ONLY` to `_LIBCPP_TEMPLATE_VIS` and committed 
this as revision 351993.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D28248/new/

https://reviews.llvm.org/D28248



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57001: [libunwind] Don't define unw_fpreg_t to uint64_t for __ARM_DWARF_EH__

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

I have no problem with the code change, but no context on whether or not it is 
correct.
I'm hoping some other people familiar with ARM and DWARF can chime in.


Repository:
  rUNW libunwind

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57001/new/

https://reviews.llvm.org/D57001



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56905: [libunwind] [SjLj] Don't use __declspec(thread) in MinGW mode

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.

Sigh. At least we've got a macro wrapping this; we only have to pore over this 
once.


Repository:
  rUNW libunwind

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56905/new/

https://reviews.llvm.org/D56905



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51762: First part of the calendar stuff

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists marked 2 inline comments as done.
mclow.lists added inline comments.



Comment at: include/chrono:2667
+#if _LIBCPP_STD_VER > 17
+constexpr chrono::day operator ""d(unsigned long long __d) noexcept
+{

EricWF wrote:
> Including this file with Clang 6.0 in C++2a mode causes a compile error 
> because of "-Wreserved-user-defined-literal". We need to wrap this block with:
> 
> ```
> #if defined(__clang__)
> #pragma clang diagnostic push
> #pragma clang diagnostic ignored "-Wreserved-user-defined-literal"
> #endif
> [...]
> #if defined(__clang__)
> #pragma clang diagnostic pop
> #endif
> ```
I did this by defining `_LIBCPP_HAS_NO_CXX20_CHRONO_LITERALS` instead


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51762/new/

https://reviews.llvm.org/D51762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53763: [libc++] [test] Fix logic error in tests; enable for MSVC previews

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.
This revision is now accepted and ready to land.

I'm a bit concerned about the `TEST_HAS_NO_SPACESHIP_OPERATOR` and how it 
tracks with `_LIBCPP_HAS_NO_SPACESHIP_OPERATOR`, but I'm not going to hold this 
up for that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53763/new/

https://reviews.llvm.org/D53763



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53956: Fix test assumption that Linux implies glibc.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

I hate this chunk of code. :-(
`TEST_HAS_C11_FEATURES` basically means that we can use C11 library features 
like `aligned_alloc`, and `timespec` etc.


Repository:
  rCXX libc++

https://reviews.llvm.org/D53956



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51762: First part of the calendar stuff

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In https://reviews.llvm.org/D51762#1266086, @NoQ wrote:

> Had to revert. Sorry! https://reviews.llvm.org/rL344580.
>
> This failure was masked by another error, so i guess it was missed.


This was supposed to be fixed by commit r344535.


https://reviews.llvm.org/D51762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51762: First part of the calendar stuff

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.

After discussions with @EricWF, I landed a modified version as revision 344529.


https://reviews.llvm.org/D51762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51762: First part of the calendar stuff

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

This is the first of part of this functionality. Some of these bits are not 
here yet.




Comment at: 
test/std/utilities/time/time.cal/time.cal.ymwd/time.cal.ymwd.members/ctor.sys_days.pass.cpp:10
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// XFAIL
+

EricWF wrote:
> What?
The `sys_days` type is not implemented yet.
That will be in the next patch.




Comment at: 
test/std/utilities/time/time.cal/time.cal.ymwdlast/time.cal.ymwdlast.nonmembers/streaming.pass.cpp:10
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// XFAIL
+

EricWF wrote:
> What?
Again; the streaming bits are not here in this patch.
Next patch.



https://reviews.llvm.org/D51762



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42242: Make libc++abi work with gcc's ARM unwind library

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists closed this revision.
mclow.lists added a comment.

Committed as revision 344152


https://reviews.llvm.org/D42242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42242: Make libc++abi work with gcc's ARM unwind library

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In https://reviews.llvm.org/D42242#1257428, @mgorny wrote:

> @mclow.lists , ping. Any chance to get a proper version upstream? I'd rather 
> not pull patches from Fedora when we can have something official.


I gave up on this patch several months ago, because no one would say how much 
work it was.
I fixed one place, and was told "here are several others".
I fixed those, and was told "I get some new errors:" (which were completely 
unrelated to the previous problems.


https://reviews.llvm.org/D42242



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46975: Implement deduction guides for `std::deque`

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.
This revision is now accepted and ready to land.

Committed as r332785


https://reviews.llvm.org/D46975



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52401: Remove redundant null pointer check in operator delete

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.

Ok. I'm fine with this. Thanks for your patience.


Repository:
  rL LLVM

https://reviews.llvm.org/D52401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52401: Remove redundant null pointer check in operator delete

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In https://reviews.llvm.org/D52401#1250552, @MaskRay wrote:

> > I seem to recall a problem with that
>
> I would like to know if your impression came from the common PWN technique 
> when the attacker found a heap buffer overflow :)


No; that's not what I'm looking into.


Repository:
  rL LLVM

https://reviews.llvm.org/D52401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52401: Remove redundant null pointer check in operator delete

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

I suspect it's fine, but I need to check some stuff on old versions of glibc (I 
seem to recall a problem with that).


Repository:
  rL LLVM

https://reviews.llvm.org/D52401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52368: [libc++abi] is_strcmp parameter to is_equal is unused for WIN32

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rCXXA libc++abi

https://reviews.llvm.org/D52368



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52368: [libc++abi] is_strcmp parameter to is_equal is unused for WIN32

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

This seems like overkill to me; why not add `(void) use_strcmp` right before 
line 67 instead?


Repository:
  rCXXA libc++abi

https://reviews.llvm.org/D52368



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44263: Implement LWG 2221 - No formatted output operator for nullptr

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

Symbol added: _ZNSt3__113basic_ostreamIcNS_11char_traitsIcEEElsEDn

  {'is_defined': True, 'type': 'FUNC', 'name': 
'_ZNSt3__113basic_ostreamIcNS_11char_traitsIcEEElsEDn'}

Symbol added: _ZNSt3__113basic_ostreamIwNS_11char_traitsIwEEElsEDn

  {'is_defined': True, 'type': 'FUNC', 'name': 
'_ZNSt3__113basic_ostreamIwNS_11char_traitsIwEEElsEDn'}

Symbol added: 
_ZNSt3__1lsIwNS_11char_traitsIwRNS_13basic_ostreamIT_T0_EES7_PKc

  {'is_defined': True, 'type': 'FUNC', 'name': 
'_ZNSt3__1lsIwNS_11char_traitsIwRNS_13basic_ostreamIT_T0_EES7_PKc'}

Symbol added: 
_ZNSt3__124__put_character_sequenceIcNS_11char_traitsIcRNS_13basic_ostreamIT_T0_EES7_PKS4_m

  {'is_defined': True, 'type': 'FUNC', 'name': 
'_ZNSt3__124__put_character_sequenceIcNS_11char_traitsIcRNS_13basic_ostreamIT_T0_EES7_PKS4_m'}


https://reviews.llvm.org/D44263



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44263: Implement LWG 2221 - No formatted output operator for nullptr

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists reopened this revision.
mclow.lists added a comment.
This revision is now accepted and ready to land.

Reverted in r342590 while I investigate additions to the dylib.


https://reviews.llvm.org/D44263



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44263: Implement LWG 2221 - No formatted output operator for nullptr

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists closed this revision.
mclow.lists added a comment.

Landed as revision 342566


https://reviews.llvm.org/D44263



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50551: [libcxx] [test] Add missing to several tests.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.
This revision is now accepted and ready to land.

LFTM


https://reviews.llvm.org/D50551



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50799: Fix for PR 38495: no longer compiles on FreeBSD, due to lack of timespec_get()

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists closed this revision.
mclow.lists added a comment.

Landed as r339816


https://reviews.llvm.org/D50799



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50815: Establish the header

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists closed this revision.
mclow.lists added a comment.

Landed as r339943


https://reviews.llvm.org/D50815



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50876: Clean up newly created header

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists closed this revision.
mclow.lists added a comment.

Landed as r340049


https://reviews.llvm.org/D50876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51868: [libcxx] Build and test fixes for Windows

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added inline comments.



Comment at: test/support/verbose_assert.h:26
   static_assert(ST == -1, "specialization required for ST != -1");
   static void Print(Tp const&) { std::clog << "Value Not Streamable!\n"; }
 };

> The renaming is to clarify that an `stderr` stream is being selected.
And yet, there is `clog`, right there.



https://reviews.llvm.org/D51868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51868: [libcxx] Build and test fixes for Windows

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added inline comments.



Comment at: test/support/verbose_assert.h:24
 : (IsStreamable::value ? 2 : -1))>
-struct SelectStream {
+struct SelectErrStream {
   static_assert(ST == -1, "specialization required for ST != -1");

Why the renaming here? 

What's the deal with sometimes using `clog` and sometimes `cerr`?
(I know, you didn't do this, but ???)



Repository:
  rCXX libc++

https://reviews.llvm.org/D51868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51955: Create infrastructure for defining and testing feature test macros

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists closed this revision.
mclow.lists added a comment.

landed as revision 342073


https://reviews.llvm.org/D51955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51955: Create infrastructure for defining and testing feature test macros

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists created this revision.
mclow.lists added reviewers: ldionne, EricWF.
Herald added a reviewer: jfb.
Herald added a subscriber: jfb.
mclow.lists edited the summary of this revision.

In P0941 , the committee standardized a bunch of 
feature test macros.
[ I think it's a bad idea, but whatever. ]

This patch puts the existing libc++ feature test macros in ``, where 
P0941 specifies, and makes sure that all the correct headers include 
``. It also introduces a full set of tests, one per header file that 
might get something defined.

At the moment, most of those tests are empty, since libc++ does not define many 
feature test macros.
But now it's easy to add more. Put them in `` and update the 
appropriate tests. Helpfully, the list is at the top of each of the test files.


https://reviews.llvm.org/D51955

Files:
  include/algorithm
  include/any
  include/array
  include/atomic
  include/bit
  include/chrono
  include/cmath
  include/complex
  include/cstddef
  include/deque
  include/exception
  include/filesystem
  include/forward_list
  include/functional
  include/iomanip
  include/iterator
  include/list
  include/map
  include/memory
  include/mutex
  include/new
  include/numeric
  include/optional
  include/regex
  include/scoped_allocator
  include/set
  include/shared_mutex
  include/string
  include/string_view
  include/tuple
  include/type_traits
  include/unordered_map
  include/unordered_set
  include/utility
  include/variant
  include/vector
  include/version
  
test/std/language.support/support.limits/support.limits.general/algorithm.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/any.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/array.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/atomic.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/bit.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/chrono.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/cmath.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/complex.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/concepts.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/cstddef.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/deque.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/exception.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/execution.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/filesystem.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/forward_list.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/functional.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/iomanip.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/iterator.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/list.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/map.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/memory.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/memory_resource.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/mutex.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/new.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/numeric.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/optional.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/regex.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/scoped_allocator.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/set.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/shared_mutex.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/string.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/string_view.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/tuple.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/type_traits.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/unordered_map.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/unordered_set.version.pass.cpp
  
test/std/language.support/support.limits/support.limits.general/utility.version.pass.cpp
  

[PATCH] D50101: [asan] Update a vector's storage annotation during destruction.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists closed this revision.
mclow.lists added a comment.

landed as revision 341671.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50101



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

I should also mention that as a conforming extension, I have implemented the 
non-numeric bit operations for `std::byte`


https://reviews.llvm.org/D51262



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists created this revision.
mclow.lists added reviewers: EricWF, ldionne.
Herald added a subscriber: christof.

LWG adopted https://wg21.link/P0553 in Rapperswil, and suggested minor changes 
to https://wg21.link/P0556.

They kind of go together; for example, `ispow2` is easily implemented using 
`popcount` - and they share a bunch of infastructure.

I don't recommend landing this until P0556 is approved, but when I implemented 
one, the other was easy.
None of this stuff will be constexpr on Windows, because the underlying 
primitives are not constexpr on Windows.

Sorry for the large-ish diff, but (like span) it's 85%+ tests.


https://reviews.llvm.org/D51262

Files:
  include/bit
  test/libcxx/numerics/bit/bitops.count/countl_one.pass.cpp
  test/libcxx/numerics/bit/bitops.count/countl_zero.pass.cpp
  test/libcxx/numerics/bit/bitops.count/countr_one.pass.cpp
  test/libcxx/numerics/bit/bitops.count/countr_zero.pass.cpp
  test/libcxx/numerics/bit/bitops.count/popcount.pass.cpp
  test/libcxx/numerics/bit/bitops.rot/rotl.pass.cpp
  test/libcxx/numerics/bit/bitops.rot/rotr.pass.cpp
  test/std/numerics/bit/bit.pow.two/ceil2.fail.cpp
  test/std/numerics/bit/bit.pow.two/ceil2.pass.cpp
  test/std/numerics/bit/bit.pow.two/floor2.fail.cpp
  test/std/numerics/bit/bit.pow.two/floor2.pass.cpp
  test/std/numerics/bit/bit.pow.two/ispow2.fail.cpp
  test/std/numerics/bit/bit.pow.two/ispow2.pass.cpp
  test/std/numerics/bit/bit.pow.two/log2p1.fail.cpp
  test/std/numerics/bit/bit.pow.two/log2p1.pass.cpp
  test/std/numerics/bit/bitops.count/countl_one.fail.cpp
  test/std/numerics/bit/bitops.count/countl_one.pass.cpp
  test/std/numerics/bit/bitops.count/countl_zero.fail.cpp
  test/std/numerics/bit/bitops.count/countl_zero.pass.cpp
  test/std/numerics/bit/bitops.count/countr_one.fail.cpp
  test/std/numerics/bit/bitops.count/countr_one.pass.cpp
  test/std/numerics/bit/bitops.count/countr_zero.fail.cpp
  test/std/numerics/bit/bitops.count/countr_zero.pass.cpp
  test/std/numerics/bit/bitops.count/popcount.fail.cpp
  test/std/numerics/bit/bitops.count/popcount.pass.cpp
  test/std/numerics/bit/bitops.rot/rotl.fail.cpp
  test/std/numerics/bit/bitops.rot/rotl.pass.cpp
  test/std/numerics/bit/bitops.rot/rotr.fail.cpp
  test/std/numerics/bit/bitops.rot/rotr.pass.cpp
  test/std/numerics/bit/nothing_to_do.pass.cpp

Index: test/std/numerics/bit/nothing_to_do.pass.cpp
===
--- test/std/numerics/bit/nothing_to_do.pass.cpp
+++ test/std/numerics/bit/nothing_to_do.pass.cpp
@@ -0,0 +1,12 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+int main()
+{
+}
Index: test/std/numerics/bit/bitops.rot/rotr.pass.cpp
===
--- test/std/numerics/bit/bitops.rot/rotr.pass.cpp
+++ test/std/numerics/bit/bitops.rot/rotr.pass.cpp
@@ -0,0 +1,137 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+// UNSUPPORTED: c++98, c++03, c++11, c++14, c++17 
+
+// template 
+//   constexpr int rotr(T x, unsigned int s) noexcept;
+
+// Remarks: This function shall not participate in overload resolution unless 
+//	T is an unsigned integer type
+
+#include 
+#include 
+
+#include "test_macros.h"
+
+template 
+constexpr bool constexpr_test()
+{
+	const T max = std::numeric_limits::max();
+
+	return std::rotr(T(128), 0) == T(128)
+	   &&  std::rotr(T(128), 1) == T( 64)
+	   &&  std::rotr(T(128), 2) == T( 32)
+	   &&  std::rotr(T(128), 3) == T( 16)
+	   &&  std::rotr(T(128), 4) == T(  8)
+	   &&  std::rotr(T(128), 5) == T(  4)
+	   &&  std::rotr(T(128), 6) == T(  2)
+	   &&  std::rotr(T(128), 7) == T(  1)
+	   &&  std::rotr(max, 0)  == max
+	   &&  std::rotr(max, 1)  == max
+	   &&  std::rotr(max, 2)  == max
+	   &&  std::rotr(max, 3)  == max
+	   &&  std::rotr(max, 4)  == max
+	   &&  std::rotr(max, 5)  == max
+	   &&  std::rotr(max, 6)  == max
+	   &&  std::rotr(max, 7)  == max
+	  ;
+}
+
+
+template 
+void runtime_test()
+{
+	ASSERT_SAME_TYPE(T, decltype(std::rotr(T(0), 0)));
+	ASSERT_NOEXCEPT( std::rotr(T(0), 0));
+	const T max = std::numeric_limits::max();
+	const T val = std::numeric_limits::max() - 1;
+
+	const T uppers [] = {
+		max,	  // not used
+		max - max,	  // 000 .. 0
+		max - (max >> 1), // 800 .. 0
+		max - (max >> 2), // C00 .. 0
+		max - (max >> 3), // E00 .. 0
+		max - (max >> 4), // 

[PATCH] D50876: Clean up newly created header

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In https://reviews.llvm.org/D50876#1204531, @mclow.lists wrote:

> @craig.topper - that's existing code; I'm not changing it.
>  If we have a test bot that I can test this against, I'm happy to update it.


I'm not really sure that this code is actually used anywhere.


https://reviews.llvm.org/D50876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50876: Clean up newly created header

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

@craig.topper - that's existing code; I'm not changing it.
If we have a test bot that I can test this against, I'm happy to update it.


https://reviews.llvm.org/D50876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50876: Clean up newly created header

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added inline comments.



Comment at: include/bit:96
 #if defined(_LIBCPP_HAS_BITSCAN64)
 (defined(_M_AMD64) || defined(__x86_64__))
+  if (_BitScanForward64(&__where, __x))

I'm not sure this code is ever used - since how can this compile?


https://reviews.llvm.org/D50876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50876: Clean up newly created header

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists updated this revision to Diff 161284.
mclow.lists added a comment.

Clean up the windows code a bit - though I don't think is used - since I don't 
think it will compile.


https://reviews.llvm.org/D50876

Files:
  include/__bit_reference
  include/bit

Index: include/bit
===
--- include/bit
+++ include/bit
@@ -35,135 +35,123 @@
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
+#ifndef _LIBCPP_COMPILER_MSVC
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __ctz(unsigned __x)   { return __builtin_ctz(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __ctz(unsigned long __x)  { return __builtin_ctzl(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __ctz(unsigned long long __x) { return __builtin_ctzll(__x); }
+
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __clz(unsigned __x)   { return __builtin_clz(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __clz(unsigned long __x)  { return __builtin_clzl(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __clz(unsigned long long __x) { return __builtin_clzll(__x); }
+
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned __x)   { return __builtin_popcount(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned long __x)  { return __builtin_popcountl(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned long long __x) { return __builtin_popcountll(__x); }
+
+#else  // _LIBCPP_COMPILER_MSVC
+
 // Precondition:  __x != 0
 inline _LIBCPP_INLINE_VISIBILITY
-unsigned __ctz(unsigned __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_ctz(__x));
-#else
+int __ctz(unsigned __x) {
   static_assert(sizeof(unsigned) == sizeof(unsigned long), "");
   static_assert(sizeof(unsigned long) == 4, "");
-  unsigned long where;
-  // Search from LSB to MSB for first set bit.
-  // Returns zero if no set bit is found.
-  if (_BitScanForward(, __x))
-return where;
+  unsigned long __where;
+  if (_BitScanForward(&__where, __x))
+return static_cast(__where);
   return 32;
-#endif
 }
 
 inline _LIBCPP_INLINE_VISIBILITY
-unsigned long __ctz(unsigned long __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_ctzl(__x));
-#else
+int __ctz(unsigned long __x) {
 static_assert(sizeof(unsigned long) == sizeof(unsigned), "");
 return __ctz(static_cast(__x));
-#endif
 }
 
 inline _LIBCPP_INLINE_VISIBILITY
-unsigned long long __ctz(unsigned long long __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_ctzll(__x));
-#else
-unsigned long where;
-// Search from LSB to MSB for first set bit.
-// Returns zero if no set bit is found.
+int __ctz(unsigned long long __x) {
+unsigned long __where;
 #if defined(_LIBCPP_HAS_BITSCAN64)
 (defined(_M_AMD64) || defined(__x86_64__))
-  if (_BitScanForward64(, __x))
-return static_cast(where);
+  if (_BitScanForward64(&__where, __x))
+return static_cast(__where);
 #else
   // Win32 doesn't have _BitScanForward64 so emulate it with two 32 bit calls.
-  // Scan the Low Word.
-  if (_BitScanForward(, static_cast(__x)))
-return where;
-  // Scan the High Word.
-  if (_BitScanForward(, static_cast(__x >> 32)))
-return where + 32; // Create a bit offset from the LSB.
+  if (_BitScanForward(&__where, static_cast(__x)))
+return static_cast(__where);
+  if (_BitScanForward(&__where, static_cast(__x >> 32)))
+return static_cast(__where + 32);
 #endif
   return 64;
-#endif // _LIBCPP_COMPILER_MSVC
 }
 
 // Precondition:  __x != 0
 inline _LIBCPP_INLINE_VISIBILITY
-unsigned __clz(unsigned __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_clz(__x));
-#else
+int __clz(unsigned __x) {
   static_assert(sizeof(unsigned) == sizeof(unsigned long), "");
   static_assert(sizeof(unsigned long) == 4, "");
-  unsigned long where;
-  // Search from LSB to MSB for first set bit.
-  // Returns zero if no set bit is found.
-  if (_BitScanReverse(, __x))
-return 31 - where;
+  unsigned long __where;
+  if (_BitScanReverse(&__where, __x))
+return static_cast(31 - __where);
   return 32; // Undefined Behavior.
-#endif
 }
 
 inline _LIBCPP_INLINE_VISIBILITY
-unsigned long __clz(unsigned long __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_clzl (__x));
-#else
+int __clz(unsigned long __x) {
 static_assert(sizeof(unsigned) == sizeof(unsigned long), "");
 return __clz(static_cast(__x));
-#endif
 }
 
 inline _LIBCPP_INLINE_VISIBILITY
-unsigned long long __clz(unsigned long long __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_clzll(__x));
-#else
-  unsigned long where;
-// BitScanReverse scans from MSB to LSB for first set bit.
-// Returns 0 if no set bit is found.
+int __clz(unsigned long long __x) {
+  unsigned long __where;
 #if defined(_LIBCPP_HAS_BITSCAN64)
-  if (_BitScanReverse64(, __x))
-return static_cast(63 - where);
+  if (_BitScanReverse64(&__where, __x))
+return static_cast(63 

[PATCH] D50876: Clean up newly created header

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added inline comments.



Comment at: include/bit:117
+  unsigned long __where;
   // Search from LSB to MSB for first set bit.
   // Returns zero if no set bit is found.

lebedev.ri wrote:
> Like i commented in the original review, this should probably be 
> ```
> // Search from *M*SB to *L*SB for first set bit.
> ```
I think my preference would be to just remove the comment.


https://reviews.llvm.org/D50876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50876: Clean up newly created header

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added inline comments.



Comment at: include/bit:113
 inline _LIBCPP_INLINE_VISIBILITY
 unsigned __clz(unsigned __x) {
   static_assert(sizeof(unsigned) == sizeof(unsigned long), "");

Missed this one. Should be `int`


https://reviews.llvm.org/D50876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50876: Clean up newly created header

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists created this revision.
mclow.lists added reviewers: ldionne, EricWF.

Still NFC here.

1. Take the 9 functions that each had an `#ifndef _LIBCPP_COMPILER_MSVC` .. 
`#else` .. `endif` block in them and make just two blocks, each with 9 
functions. Much easier to read, but makes for a terrible diff.

2. Change the return type of `__ctz` and `__clz` from the same type as the 
parameter to `int`.  I would have preferred `unsigned`, but that's not what 
P0553 gave us. I reviewed all the call sites for these functions, and for 
`__ctz` it was always immediately static_cast-ed to another type, and `__clz` 
it made no difference either.

3. Change the name `__pop_count` to `__popcount` to match the name that we're 
going to add from P0553.

4. Rename the local variable in the windows code from `where` to `__where`. 
Shame on someone ;-)


https://reviews.llvm.org/D50876

Files:
  include/__bit_reference
  include/bit

Index: include/bit
===
--- include/bit
+++ include/bit
@@ -35,135 +35,134 @@
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
+#ifndef _LIBCPP_COMPILER_MSVC
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __ctz(unsigned __x)   { return __builtin_ctz(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __ctz(unsigned long __x)  { return __builtin_ctzl(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __ctz(unsigned long long __x) { return __builtin_ctzll(__x); }
+
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __clz(unsigned __x)   { return __builtin_clz(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __clz(unsigned long __x)  { return __builtin_clzl(__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __clz(unsigned long long __x) { return __builtin_clzll(__x); }
+
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned __x)   { return __builtin_popcount  (__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned long __x)  { return __builtin_popcountl (__x); }
+
+inline _LIBCPP_INLINE_VISIBILITY
+int __popcount(unsigned long long __x) { return __builtin_popcountll(__x); }
+
+#else  // _LIBCPP_COMPILER_MSVC
+
 // Precondition:  __x != 0
 inline _LIBCPP_INLINE_VISIBILITY
-unsigned __ctz(unsigned __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_ctz(__x));
-#else
+int __ctz(unsigned __x) {
   static_assert(sizeof(unsigned) == sizeof(unsigned long), "");
   static_assert(sizeof(unsigned long) == 4, "");
-  unsigned long where;
+  unsigned long __where;
   // Search from LSB to MSB for first set bit.
   // Returns zero if no set bit is found.
-  if (_BitScanForward(, __x))
-return where;
+  if (_BitScanForward(&__where, __x))
+return static_cast(__where);
   return 32;
-#endif
 }
 
 inline _LIBCPP_INLINE_VISIBILITY
-unsigned long __ctz(unsigned long __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_ctzl(__x));
-#else
+int __ctz(unsigned long __x) {
 static_assert(sizeof(unsigned long) == sizeof(unsigned), "");
 return __ctz(static_cast(__x));
-#endif
 }
 
 inline _LIBCPP_INLINE_VISIBILITY
-unsigned long long __ctz(unsigned long long __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_ctzll(__x));
-#else
-unsigned long where;
+int __ctz(unsigned long long __x) {
+unsigned long __where;
 // Search from LSB to MSB for first set bit.
 // Returns zero if no set bit is found.
 #if defined(_LIBCPP_HAS_BITSCAN64)
 (defined(_M_AMD64) || defined(__x86_64__))
-  if (_BitScanForward64(, __x))
-return static_cast(where);
+  if (_BitScanForward64(&__where, __x))
+return static_cast(__where);
 #else
   // Win32 doesn't have _BitScanForward64 so emulate it with two 32 bit calls.
   // Scan the Low Word.
-  if (_BitScanForward(, static_cast(__x)))
-return where;
+  if (_BitScanForward(&__where, static_cast(__x)))
+return __where;
   // Scan the High Word.
-  if (_BitScanForward(, static_cast(__x >> 32)))
-return where + 32; // Create a bit offset from the LSB.
+  if (_BitScanForward(&__where, static_cast(__x >> 32)))
+return __where + 32; // Create a bit offset from the LSB.
 #endif
   return 64;
-#endif // _LIBCPP_COMPILER_MSVC
 }
 
 // Precondition:  __x != 0
 inline _LIBCPP_INLINE_VISIBILITY
 unsigned __clz(unsigned __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_clz(__x));
-#else
   static_assert(sizeof(unsigned) == sizeof(unsigned long), "");
   static_assert(sizeof(unsigned long) == 4, "");
-  unsigned long where;
+  unsigned long __where;
   // Search from LSB to MSB for first set bit.
   // Returns zero if no set bit is found.
-  if (_BitScanReverse(, __x))
-return 31 - where;
+  if (_BitScanReverse(&__where, __x))
+return 31 - __where;
   return 32; // Undefined Behavior.
-#endif
 }
 
 inline _LIBCPP_INLINE_VISIBILITY
-unsigned long __clz(unsigned long __x) {
-#ifndef _LIBCPP_COMPILER_MSVC
-return static_cast(__builtin_clzl (__x));
-#else
+int __clz(unsigned long __x) {