Page MenuHomePhabricator

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

Authored by ddcc on Aug 15 2022, 4:34 PM.

Details

Reviewers
ldionne
philnik
Group Reviewers
Restricted Project
Summary

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

Diff Detail

Event Timeline

ddcc created this revision.Aug 15 2022, 4:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2022, 4:34 PM
ddcc requested review of this revision.Aug 15 2022, 4:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2022, 4:34 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik requested changes to this revision.Aug 16 2022, 2:31 AM
philnik added a subscriber: philnik.

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

libcxx/include/cmath
534

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?

This revision now requires changes to proceed.Aug 16 2022, 2:31 AM

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 marked an inline comment as not done.Aug 16 2022, 2:48 PM

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
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 updated this revision to Diff 453137.Aug 16 2022, 2:54 PM
ddcc marked an inline comment as not done.

Update test suite flags, use numeric_limits

ddcc added inline comments.Aug 16 2022, 3:30 PM
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 added inline comments.Aug 16 2022, 3:32 PM
libcxx/include/cmath
534

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 added inline comments.Aug 16 2022, 4:28 PM
libcxx/utils/libcxx/test/params.py
21 ↗(On Diff #453137)

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 updated this revision to Diff 453158.Aug 16 2022, 4:30 PM

Handle string and variant