[libcxx-commits] [PATCH] D121552: [libc++] Add warning pragma macros in the test suite

Sun Mar 13 17:21:07 PDT 2022

EricWF added a comment.

Have @ldionnes changes to enable warnings in the test suite been submitted yet? Maybe that's why you're not seeing some of the warnings you removed.
Can you double check that (A) The headers aren't included with `-isystem` or `-cxx-isystem` and that `_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER` is defined
when you've tested this?

In D121552#3378178 <https://reviews.llvm.org/D121552#3378178>, @philnik wrote:

> In D121552#3378148 <https://reviews.llvm.org/D121552#3378148>, @EricWF wrote:
>
>> Every single warning we disable should come with a comment explaining why the warning isn't real and why we're not fixing it some other way. We shouldn't encourage suppressing warnings.
>
> I mostly agree, but I don't think this is the PR for that.

I disagree. Changing the code changes the git blame and makes it harder to do archology on it.
If you're modifying the code in this patch, it's the time to ensure it has a comment.

That said, almost all of the changes already do have comments.:-D

================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.rel/three_way.pass.cpp:22
+TEST_CLANG_DIAGNOSTIC_IGNORED("-Wsign-compare")
+TEST_GCC_DIAGNOSTIC_IGNORED("-Wsign-compare")
+TEST_MSVC_DIAGNOSTIC_IGNORED(4242 4244)
----------------

Clang respects GCC's pragmas. We don't need both here. 

================
Comment at: libcxx/test/support/unique_ptr_test_helper.h:137

-#if defined(__GNUC__)
-#pragma GCC diagnostic push
----------------
I think this needs to stay.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121552