Fix warnings when compiling libcxx for Windows with clang-cl /W4

[ColinFinck]

Summary

Only these two unused variables left, then it's perfect.

Colin Finck <[e-mail]>

[mstorsjo]

Hmm, when I build libcxx with clang-cl, the build is extremely noisy, with around 100 warnings per source file. These mostly come from -Wall which the build system adds, warning for things in the libcxx headers (which the compiler doesn't consider to be system headers at that point and doesn't mute warnings for). What's your build setup like, do you manage to avoid getting -Wall added, or is it ignoring things in the headers in your case?

[ColinFinck]

I'm also getting a very noisy build with -Wall, but here I'm using /W4 (building libc++ in a custom Visual Studio project file).
There are plenty of other occasions in libc++ where unused parameter warnings are prevented just like I did in my patch. Which is why I hope, we can shortly fix the last 2 remaining ones.

[ColinFinck]

What is blocking this?
I'm just silencing two unused parameter warnings the same way this is done in other parts of the libc++ code.

[curdeius]

Just the review throughput and the fact that some reviews get forgotten. Next time you may ping earlier of you see no activity.
Anyway, LGTM.

[• Quuxplusone]

libcxx/src/atomic.cpp

154

The comment on line 123 becomes misleading. Here I suggest following line 86's style:

static __cxx_contention_t __libcpp_contention_monitor_for_wait(__cxx_atomic_contention_t volatile*,
                                                               __cxx_atomic_contention_t const volatile* __platform_state)
{
    // We will monitor this value.
    return __cxx_atomic_load(__platform_state, memory_order_acquire);
}

IMO there should also be one blank line before this function, and one blank line after.

libcxx/src/support/win32/locale_win32.cpp

23

Here we have two unused parameters: mask and base. I'd prefer to handle them both in the same way. I suggest

// FIXME: base and mask currently unused. Needs manual work to construct the new locale
locale_t newlocale(int /*mask*/, const char *locale, locale_t /*base*/)
{
    return {_create_locale( LC_ALL, locale ), locale};
}

[mstorsjo]

FWIW, when building with CMake, we already do try to add -Wno-unused-parameter (which ends up applied also when building with clang-cl), so in that sense these shouldn't be needed when building in such configurations. But if we have a precedent for silencing this kind of unused parameters like this too (which is quite harmless anyway), for silencing them when building libcxx via other custom build systems, then that's certainly good too.

[ColinFinck]

Apply suggestions from @Quuxplusone

[ColinFinck]

@Quuxplusone Is this ready to be merged now?

[• Quuxplusone]

Sure, LGTM. I forget if @curdeius counts as quorum for libc++ approvers, so I'm going to just mark this "Accepted" for me personally, and let someone else (@ldionne or @Mordante) give the final libc++ approval.

[ldionne]

In D96408#3076537, @Quuxplusone wrote:

Sure, LGTM. I forget if @curdeius counts as quorum for libc++ approvers, so I'm going to just mark this "Accepted" for me personally, and let someone else (@ldionne or @Mordante) give the final libc++ approval.

Yes. You can see members of the libc++ review group here: https://reviews.llvm.org/project/members/64/

libcxx/src/atomic.cpp

 static void __libcpp_contention_notify(__cxx_atomic_contention_t volatile* __contention_state,
                                        __cxx_atomic_contention_t const volatile* __platform_state,
                                        bool __notify_one)
 {
     if(0 != __cxx_atomic_load(__contention_state, memory_order_seq_cst))
         // We only call 'wake' if we consumed a contention bit here.
         __libcpp_platform_wake_by_address(__platform_state, __notify_one);
 }
-static __cxx_contention_t __libcpp_contention_monitor_for_wait(__cxx_atomic_contention_t volatile* __contention_state,
+static __cxx_contention_t __libcpp_contention_monitor_for_wait(__cxx_atomic_contention_t volatile* /*__contention_state*/,
                                                                __cxx_atomic_contention_t const volatile* __platform_state)
 {
     // We will monitor this value.
     return __cxx_atomic_load(__platform_state, memory_order_acquire);
 }
 static void __libcpp_contention_wait(__cxx_atomic_contention_t volatile* __contention_state,
                                      __cxx_atomic_contention_t const volatile* __platform_state,
                                      __cxx_contention_t __old_value)
 {
     __cxx_atomic_fetch_add(__contention_state, __cxx_contention_t(1), memory_order_seq_cst);
     // We sleep as long as the monitored value hasn't changed.
     __libcpp_platform_wait_on_address(__platform_state, __old_value);

libcxx/src/support/win32/locale_win32.cpp

 #include <type_traits>
 
 #include <__locale_dir/locale_base_api/locale_guard.h>
 
 int __libcpp_vasprintf(char **sptr, const char *__restrict fmt, va_list ap);
 
 using std::__libcpp_locale_guard;
 
-// FIXME: base currently unused. Needs manual work to construct the new locale
-locale_t newlocale( int mask, const char * locale, locale_t /*base*/ )
+// FIXME: base and mask currently unused. Needs manual work to construct the new locale
+locale_t newlocale(int /*mask*/, const char * locale, locale_t /*base*/)
 {
     return {_create_locale( LC_ALL, locale ), locale};
 }
 
 decltype(MB_CUR_MAX) MB_CUR_MAX_L( locale_t __l )
 {
 #if defined(_LIBCPP_MSVCRT)
   return ___mb_cur_max_l_func(__l);
 #else
   __libcpp_locale_guard __current(__l);