[libcxx] Resolve warnings for Wimplicit-float-conversion, Wmacro-redefined, Wzero-as-null-pointer-constant, Wsign-conversion

[Mordante]

Summary

These warning were identified while debugging modules with Wsystem-headers.

[philnik]

These warnings should be enabled in the test suite to avoid regressions.

libcxx/include/cmath

534–535

What exactly is the problem here? sqrt has overloads for float, double and long double.

libcxx/include/string_view

288

Maybe use numeric_limits here instead?

[ldionne]

Thanks for the patch!

These warnings should be enabled in the test suite to avoid regressions.

@ddcc You can do that by adding them in libcxx/utils/libcxx/test/params.py.

libcxx/include/atomic

2694

Is this because ATOMIC_FLAG_INIT was already defined by the C Library?

[ddcc]

In D131929#3727098, @ldionne wrote:

Thanks for the patch!

These warnings should be enabled in the test suite to avoid regressions.

@ddcc You can do that by adding them in libcxx/utils/libcxx/test/params.py.

Will do.

libcxx/include/atomic

2694

Yeah, not sure whether we want to unconditionally undef or do ifdef here.

libcxx/include/cmath

534–535

warning: implicit conversion loses floating-point precision: 'double' to 'float' [-Wimplicit-float-conversion]
inline _LIBCPP_INLINE_VISIBILITY float       hypot(       float x,       float y,       float z ) { return sqrt(x*x + y*y + z*z); }
                                                                                                    ~~~~~~ ^~~~~~~~~~~~~~~~~~~~~

libcxx/include/string_view

288

Will do.

[ddcc]

Update test suite flags, use numeric_limits

[ddcc]

libcxx/include/string_view

288

Hmm looks like numeric_limits just moves the problem:

warning: signed shift result (0x8000000000000000) sets the sign bit of the shift expression's type ('long') and becomes negative [-Wshift-sign-overflow]
    static _LIBCPP_CONSTEXPR const _Tp value = _Tp(_Tp(1) << __digits);

Two alternatives come to mind; either we suppress the warning here, or we do something similar to the following (for e.g. int64_t):

-static_cast<uint64_t>((1ULL << (sizeof(int64_t) * CHAR_BITS - 1) - 1)

[ddcc]

libcxx/include/cmath

534–535

I'm not quite sure whats going on here, I get a bunch of these even with my changes:

warning: implicit conversion loses floating-point precision: 'double' to 'float' [-Wimplicit-float-conversion]
inline _LIBCPP_INLINE_VISIBILITY float       hypot(       float __x,       float __y,       float __z ) { return sqrt(__x*__x + __y*__y + __z*__z); }
                                                                                                          ~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
warning: implicit conversion increases floating-point precision: 'float' to 'double' [-Wdouble-promotion]
inline _LIBCPP_INLINE_VISIBILITY float       hypot(       float __x,       float __y,       float __z ) { return sqrt(__x*__x + __y*__y + __z*__z); }

[ddcc]

libcxx/utils/libcxx/test/params.py

21

Don't think I can make this change yet. There's other related warnings:

atomic: warning: implicit conversion changes signedness: 'std::__memory_order_underlying_t' (aka 'unsigned int') to 'int' [-Wsign-conversion]
    __c11_atomic_signal_fence(static_cast<__memory_order_underlying_t>(__order));

I don't quite understand this bit, there's some comments about conditional behavior under _LIBCPP_STD_VER > 17

[ddcc]

Handle string and variant

[Mordante]

We're moving to GitHub PRs it would be great when this patch can be finished before moving. Are you interested in finishing it? If not I'll take over. Please start by rebasing the patch to see whether it passes the CI.

[ddcc]

I don't have the time right now, feel free to take it over.

[Mordante]

In D131929#4637843, @ddcc wrote:

I don't have the time right now, feel free to take it over.

Thanks, I'll take it over.

[Mordante]

Rebased.

[Mordante]

CI fixes.

[Mordante]

Retrigger CI.

[Mordante]

Trigger CI.

libcxx/include/atomic

 // general atomic<T>
 
 template <class _Tp, bool = is_integral<_Tp>::value && !is_same<_Tp, bool>::value>
 struct __atomic_base  // false
 {
     mutable __cxx_atomic_impl<_Tp> __a_;
 
 #if defined(__cpp_lib_atomic_is_always_lock_free)
-  static _LIBCPP_CONSTEXPR bool is_always_lock_free = __atomic_always_lock_free(sizeof(__a_), 0);
+  static _LIBCPP_CONSTEXPR bool is_always_lock_free = __atomic_always_lock_free(sizeof(__a_), nullptr);
 #endif
 
     _LIBCPP_INLINE_VISIBILITY
     bool is_lock_free() const volatile _NOEXCEPT
         {return __cxx_atomic_is_lock_free(sizeof(_Tp));}
     _LIBCPP_INLINE_VISIBILITY
     bool is_lock_free() const _NOEXCEPT
         {return static_cast<__atomic_base const volatile*>(this)->is_lock_free();}

 typedef atomic<size_t>    atomic_size_t;
 typedef atomic<ptrdiff_t> atomic_ptrdiff_t;
 typedef atomic<intmax_t>  atomic_intmax_t;
 typedef atomic<uintmax_t> atomic_uintmax_t;
 
 // atomic_*_lock_free : prefer the contention type most highly, then the largest lock-free type
 
 #ifdef __cpp_lib_atomic_is_always_lock_free
-# define _LIBCPP_CONTENTION_LOCK_FREE __atomic_always_lock_free(sizeof(__cxx_contention_t), 0)
+# define _LIBCPP_CONTENTION_LOCK_FREE __atomic_always_lock_free(sizeof(__cxx_contention_t), nullptr)
 #else
 # define _LIBCPP_CONTENTION_LOCK_FREE false
 #endif
 
 #if ATOMIC_LLONG_LOCK_FREE == 2
 typedef conditional<_LIBCPP_CONTENTION_LOCK_FREE, __cxx_contention_t, long long>::type          __libcpp_signed_lock_free;
 typedef conditional<_LIBCPP_CONTENTION_LOCK_FREE, __cxx_contention_t, unsigned long long>::type __libcpp_unsigned_lock_free;
 #elif ATOMIC_INT_LOCK_FREE == 2

 #define _LIBCPP_NO_LOCK_FREE_TYPES
 #endif
 
 #if !defined(_LIBCPP_NO_LOCK_FREE_TYPES)
 typedef atomic<__libcpp_signed_lock_free> atomic_signed_lock_free;
 typedef atomic<__libcpp_unsigned_lock_free> atomic_unsigned_lock_free;
 #endif
 
+#undef ATOMIC_FLAG_INIT
 #define ATOMIC_FLAG_INIT {false}
+#undef ATOMIC_VAR_INIT
 #define ATOMIC_VAR_INIT(__v) {__v}
 
 #if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_DISABLE_DEPRECATION_WARNINGS)
 # if defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER >= 1400
 #  pragma clang deprecated(ATOMIC_VAR_INIT)
 # endif
 #endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_DISABLE_DEPRECATION_WARNINGS)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP_ATOMIC

libcxx/include/cmath

 using ::roundl _LIBCPP_USING_IF_EXISTS;
 using ::scalblnl _LIBCPP_USING_IF_EXISTS;
 using ::scalbnl _LIBCPP_USING_IF_EXISTS;
 using ::tgammal _LIBCPP_USING_IF_EXISTS;
 using ::truncl _LIBCPP_USING_IF_EXISTS;
 
 #if _LIBCPP_STD_VER > 14
 inline _LIBCPP_INLINE_VISIBILITY float       hypot(       float __x,       float __y,       float __z ) { return sqrt(__x*__x + __y*__y + __z*__z); }
-inline _LIBCPP_INLINE_VISIBILITY double      hypot(      double __x,      double __y,      double __z ) { return sqrt(__x*__x + __y*__y + __z*__z); }
-inline _LIBCPP_INLINE_VISIBILITY long double hypot( long double __x, long double __y, long double __z ) { return sqrt(__x*__x + __y*__y + __z*__z); }
+inline _LIBCPP_INLINE_VISIBILITY double      hypot(      double __x,      double __y,      double __z ) { return sqrtf(__x*__x + __y*__y + __z*__z); }
+inline _LIBCPP_INLINE_VISIBILITY long double hypot( long double __x, long double __y, long double __z ) { return sqrtl(__x*__x + __y*__y + __z*__z); }
 
 template <class _A1, class _A2, class _A3>
 inline _LIBCPP_INLINE_VISIBILITY
 typename enable_if_t
 <
     is_arithmetic<_A1>::value &&
     is_arithmetic<_A2>::value &&
     is_arithmetic<_A3>::value,

libcxx/include/string_view

public:
     typedef _CharT&                                    reference;
     typedef const _CharT&                              const_reference;
     typedef const_pointer                              const_iterator; // See [string.view.iterators]
     typedef const_iterator                             iterator;
     typedef _VSTD::reverse_iterator<const_iterator>    const_reverse_iterator;
     typedef const_reverse_iterator                     reverse_iterator;
     typedef size_t                                     size_type;
     typedef ptrdiff_t                                  difference_type;
-    static _LIBCPP_CONSTEXPR const size_type npos = -1; // size_type(-1);
+    static _LIBCPP_CONSTEXPR const size_type npos = numeric_limits<size_type>::max(); // size_type(-1);
 
     static_assert((!is_array<value_type>::value), "Character type of basic_string_view must not be an array");
     static_assert(( is_standard_layout<value_type>::value), "Character type of basic_string_view must be standard-layout");
     static_assert(( is_trivial<value_type>::value), "Character type of basic_string_view must be trivial");
     static_assert((is_same<_CharT, typename traits_type::char_type>::value),
                   "traits_type::char_type must be the same type as CharT");
 
     // [string.view.cons], construct/copy

libcxx/utils/libcxx/test/params.py

 import re
 
 _warningFlags = [
   '-Werror',
   '-Wall',
   '-Wextra',
   '-Wshadow',
   '-Wundef',
+  '-Wimplicit-float-conversion',
+  '-Wmacro-redefined',
+  '-Wsign-conversion',
+  '-Wzero-as-null-pointer-constant',
   '-Wno-unused-command-line-argument',
   '-Wno-attributes',
   '-Wno-pessimizing-move',
   '-Wno-c++11-extensions',
   '-Wno-noexcept-type',
   '-Wno-aligned-allocation-unavailable',
   '-Wno-atomic-alignment',