Page MenuHomePhabricator

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

Authored by ColinFinck on Feb 10 2021, 4:37 AM.

Details

Reviewers
curdeius
Quuxplusone
ldionne
Group Reviewers
Restricted Project
Summary

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

Colin Finck <colin@reactos.org>

Diff Detail

Event Timeline

ColinFinck requested review of this revision.Feb 10 2021, 4:37 AM
ColinFinck created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptFeb 10 2021, 4:37 AM

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?

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.

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 accepted this revision as: curdeius.Mar 8 2021, 4:06 AM
curdeius added a subscriber: 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 requested changes to this revision.Mar 8 2021, 5:33 AM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
libcxx/src/atomic.cpp
124

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
22

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};
}
This revision now requires changes to proceed.Mar 8 2021, 5:33 AM

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.

Apply suggestions from @Quuxplusone

ColinFinck marked 2 inline comments as done.Oct 19 2021, 1:15 AM

@Quuxplusone Is this ready to be merged now?

Quuxplusone added subscribers: Mordante, ldionne.

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 accepted this revision.Oct 21 2021, 6:58 AM

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/

This revision is now accepted and ready to land.Oct 21 2021, 6:58 AM