This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Guard warning pragmas
ClosedPublic

Authored by philnik on Feb 8 2022, 3:26 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGa7c2a6289c22: [libc++] Guard warning pragmas
Summary

This makes the GCC output even cleaner!

Diff Detail

Event Timeline

philnik requested review of this revision.Feb 8 2022, 3:26 PM
philnik created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2022, 3:26 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Can you describe the difference in output?

Can you describe the difference in output?

This suppresses -Wunknown-pragma. GCC doesn't know #pragma clang *. This way we also don't have to have push and pop warning macros for every compiler we support everywhere.

Mordante added inline comments.
libcxx/include/__config
1410

I feel this is a bit misleading. This diagnostic is ignored on both GCC and Clang. Clang supports the gcc diagnostics so that works. However I wonder what happens when a GCC only compiler diagnostic is pushed.

Wouldn't it be better to have
_LIBCPP_CLANG_DIAGNOSTIC_IGNORED, _LIBCPP_GCC_DIAGNOSTIC_IGNORED, and _LIBCPP_COMPILER_DIAGNOSTIC_IGNORED?
Where _LIBCPP_COMPILER_DIAGNOSTIC_IGNORED uses GCC diagnostic ignored warning.

That name is still wrong when using other compilers, but that feels better than the current naming.

philnik added inline comments.Feb 9 2022, 11:10 AM
libcxx/include/__config
1410

I also thought about the naming. _LIBCPP_GCC_DIAGNOSTIC_IGNORED didn't really feel right. Would _LIBCPP_CLANG_GCC_DIAGNOSTIC_IGNORED be better? And then _LIBCPP_GCC_DIAGNOSTIC_IGNORED only enabled on GCC.
I think _LIBCPP_COMPILER_DIAGNOSTIC_IGNORED is to general of a name. That sounds to me to much like some magic attribute that all compilers have to support (which would actually be great to have). @ldionne do you have any opinions here?

ldionne added inline comments.Feb 11 2022, 6:56 AM
libcxx/include/__config
1402

I don't feel that _LIBCPP_PRAGMA is *that* useful, since we don't actually save any typing below?

1405

We can use _LIBCPP_COMPILER_CLANG_BASED here, and _LIBCPP_COMPILER_GCC below.

1410

IMO, it would make sense to do as if Clang and GCC were completely non-overlapping here. In other words, in the code, we'd have:

_LIBCPP_DIAGNOSTIC_PUSH();
_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wunused-private-field");
_LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wunused-private-field");

    // code

_LIBCPP_DIAGNOSTIC_POP();

And one of _LIBCPP_CLANG_DIAGNOSTIC_IGNORED and _LIBCPP_GCC_DIAGNOSTIC_IGNORED would always expand to nothing. That way, we won't be lying about what _LIBCPP_<compiler>_DIAGNOSTIC_IGNORED is doing. It's a tiny bit more work when using it (because you might have to use both the macro for Clang and for GCC), but we're still improving on the status quo where we have enclosing #ifs.

libcxx/src/locale.cpp
52

Do you really need to push/pop here? And also in the other .cpp files? It seems to buy us little, since we're disabling those warnings pretty much for the whole file anyway.

Quuxplusone added inline comments.
libcxx/include/__config
1403

Neither _LIBCPP_PUSH_MACROS nor _LIBCPP_BEGIN_NAMESPACE_STD end with the noise characters ();, and I don't think _LIBCPP_DIAGNOSTIC_PUSH or _LIBCPP_DIAGNOSTIC_POP should, either. Especially since you have to go out of your way with this static_assert hack in order to make the (); syntactically valid C++ code.
Just do

#define _LIBCPP_TOSTRING2(w) #w
#define _LIBCPP_DIAGNOSTIC_PUSH _Pragma("clang diagnostic push")
#define _LIBCPP_CLANG_DIAGNOSTIC_IGNORED(w) _Pragma(_LIBCPP_TOSTRING2(clang diagnostic ignored w))
#define _LIBCPP_DIAGNOSTIC_POP _Pragma("clang diagnostic pop")

(Notice that in this branch we know we're on Clang, so we know _Pragma is supported. I initially agreed with Louis that the extra indirection of _LIBCPP_PRAGMA didn't seem to be buying us anything... but then I realized that what it was actually buying us was stringification! Which is important in this case, but _LIBCPP_PRAGMA is the wrong name for it, and in fact <__config> already provides _LIBCPP_TOSTRING2 ready-made.)

D119134 has landed now, which should be included in this one when rebasing - sorry for the conflicting change!

philnik updated this revision to Diff 408245.Feb 13 2022, 2:57 AM
philnik marked 5 inline comments as done.
  • Address comments
philnik marked an inline comment as done.Feb 13 2022, 2:57 AM
libcxx/include/__config
1405

Naming nit: I'd prefer the macro parameter be named w or x or wOption or wo or str or anything that doesn't camouflage itself so well into the phrase "clang diagnostic ignored warning". Note I believe "clang diagnostic warning ..." is also a thing.
(This applies to all 6 instances of warning.)

1415–1416

Nit: Swap these two lines to keep _LIBCPP_GCC_DIAGNOSTIC_IGNORED and _LIBCPP_CLANG_DIAGNOSTIC_IGNORED in a consistent order in all three branches.

libcxx/src/future.cpp
32–35

Please keep the PUSH and POP here. I see this is in a .cpp file, so the setting won't "leak" out into unrelated files the way it would in a .h file; but still, let's be consistent. Mainly for consistency. Hypothetically so that we don't accidentally disable this warning for switch statements that might one day be added further down in this .cpp file.

libcxx/src/hash.cpp
14

I'd like this to be scoped with PUSH and POP too, right around the lines that actually trigger the diagnostic. But what you've got here preserves the current (suboptimal) behavior, and finding out which lines trigger the diagnostic might be a research project, I dunno, so no action required here (but I think it'd be nice to have).

libcxx/src/locale.cpp
52

I commented in future.cpp with the opposite opinion: I think we should stay in the habit of tightly scoping these workarounds, and using them consistently with PUSH and POP. This makes it easier to look at the code and say "yep, that looks right" without having to worry about "well, it looks wrong, but I see it's (currently) in a .cpp file, so actually it's okay" and so on. We've picked a pattern for disabling warnings; let's stick to it.

Lines 5202 and 5348 are particularly weird: If these aren't tightly scoped workarounds for specific lines of code, then why do we wait more than 5000 lines into the file before doing them? Either these lines are meant to apply to the entire file (in which case they should be at the top of the file and/or in the build system), or they were meant to be tightly scoped workarounds and the programmer simply forgot the PUSH and POP. We shouldn't write code that looks like a mistake.

philnik updated this revision to Diff 408374.Feb 14 2022, 4:11 AM
philnik marked 3 inline comments as done.
  • Address comments

Can you include the new pragmas from 8cffea061afc556332a86fbf80ef5613db994974 / D119134 (libcxx/src/support/win32/locale_win32.cpp and libcxx/src/support/win32/support.cpp) in this cleanup too?

philnik updated this revision to Diff 408381.Feb 14 2022, 5:03 AM

@mstorsjo thanks for the reminder!

  • Rebased
philnik updated this revision to Diff 408382.Feb 14 2022, 5:09 AM
  • Replace locale_win32.cpp pragmas
ldionne requested changes to this revision.Feb 14 2022, 8:10 AM

The direction LGTM but I see a couple of easy to fix problems with the current state of the patch, so I'm requesting changes.

libcxx/include/__config
145–152

I don't think this is intended to be part of this diff. This looks like a rebase issue to me.

1427

This should use _LIBCPP_TOSTRING, not _LIBCPP_TOSTRING2 (ditto below).

libcxx/src/locale.cpp
5203

Don't we need to also add _LIBCPP_GCC_DIAGNOSTIC_IGNORED? In the "before" diff, there were branches for both GCC and Clang. But don't add it if it's not necessary!

This revision now requires changes to proceed.Feb 14 2022, 8:10 AM
philnik updated this revision to Diff 408470.Feb 14 2022, 9:53 AM
philnik marked 4 inline comments as done.
  • Rebased
  • Address comments
libcxx/src/hash.cpp
14

I will probably look into making the scope for the diagnostic suppression smaller, but not in this PR.

libcxx/src/locale.cpp
5203

I get no warnings from GCC, so I guess the warning is currently not enable for GCC. We can add the warning suppression again if we decide to enable the warning for GCC. But right now I want GCC to be able to build libc++ with -Werror.

ldionne accepted this revision.Feb 14 2022, 3:21 PM
This revision is now accepted and ready to land.Feb 14 2022, 3:21 PM
This revision was automatically updated to reflect the committed changes.