This is an archive of the discontinued LLVM Phabricator instance.

Emit an error when include <atomic> after <stdatomic.h>
ClosedPublic

Authored by vsapsai on Apr 9 2018, 6:12 PM.

Diff Detail

Repository
rC Clang

Event Timeline

vsapsai created this revision.Apr 9 2018, 6:12 PM

In addition to the currently proposed approach

#if ...
#error ...
#endif
// header content

I also tried

#if ...
#error ...
#else
// header content
#endif

It allows to have fewer error messages but can be more surprising. It can look like the only problem is a missing macro but after defining it you can discover that the situations is more complex. Also in some cases omitting a header content can lead to other errors which can be confusing too.

mclow.lists added inline comments.Apr 24 2018, 9:00 AM
clang/test/Headers/stdatomic.cpp
4 ↗(On Diff #141775)

Is there a reason we want to test this twice - once in clang and once in libc++?
We can use expected-error in libc++ tests to check the error.

vsapsai planned changes to this revision.Apr 24 2018, 12:15 PM
vsapsai added inline comments.
clang/test/Headers/stdatomic.cpp
4 ↗(On Diff #141775)

The idea was to test <stdatomic.h> in clang and <atomic> in libc++. Does that answer your question? I'm not sure I understood it correctly.

Good suggestion to use expected-error in libc++ test, will add that.

vsapsai updated this revision to Diff 143817.Apr 24 2018, 3:00 PM
  • Tighten up a test with expected-error per review comment.
mclow.lists accepted this revision.May 1 2018, 9:37 AM
This revision is now accepted and ready to land.May 1 2018, 9:37 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the review, Marshall.

In the end I removed XFAIL: with_system_cxx_lib from libc++ test as the change is header-only at compile time. Library behaviour at runtime shouldn't be affected.

jfb added a subscriber: jfb.May 3 2018, 2:15 PM

This isn't bad, so I'd go with it, but separately I imagine that we could implement the suggestion in http://wg21.link/p0943 and expose it even before C++20? Not sure we do this much, but I'd argue that before that fix stdatomic.h is just useless anyways, so it's a fine breakage. I imagine that the stdatomic.h where it's implemented would be the one injected by clang, not the libc++ one?

In D45470#1087026, @jfb wrote:

This isn't bad, so I'd go with it, but separately I imagine that we could implement the suggestion in http://wg21.link/p0943 and expose it even before C++20? Not sure we do this much, but I'd argue that before that fix stdatomic.h is just useless anyways, so it's a fine breakage. I imagine that the stdatomic.h where it's implemented would be the one injected by clang, not the libc++ one?

Change visible here is for the header injected by clang. libc++ change is r331379 but that error triggers only when you consciously decided to opt in C atomics but included C++ <atomic>.

I am concerned that the change adds the error to the valid code that uses C atomics from C++ and doesn't mix them with C++ atomics. It is unfortunate but I think it is a right trade-off. Adding __ALLOW_STDC_ATOMICS_IN_CXX__ to fix such code shouldn't take much time. While having an explicit error can save a lot of time trying to figure out a broken build. Also I expect that with time libc++ will use <atomic> more and mixing currently safe parts of libc++ with <stdatomic.h> is likely to cause problems at some point. So it is better to be prepared and have an explicit error for this case.

vsapsai reopened this revision.May 8 2018, 5:07 PM

__ALLOW_STDC_ATOMICS_IN_CXX__ approach didn't work in practice, I've reverted all changes.

This revision is now accepted and ready to land.May 8 2018, 5:07 PM
vsapsai updated this revision to Diff 145817.May 8 2018, 5:07 PM

Here is another approach that should emit an error only when mixing headers
causes compilation problems.

Have no ideas how to test the change. -verify doesn't work with fatal errors
and libcxx doesn't use FileCheck. Performed only manual testing.

vsapsai requested review of this revision.May 8 2018, 5:08 PM
vsapsai retitled this revision from Emit an error when mixing <stdatomic.h> and <atomic> to Emit an error when include <atomic> after <stdatomic.h>.
vsapsai edited the summary of this revision. (Show Details)
jfb added a comment.May 8 2018, 5:25 PM

Here is another approach that should emit an error only when mixing headers
causes compilation problems.

Have no ideas how to test the change. -verify doesn't work with fatal errors
and libcxx doesn't use FileCheck. Performed only manual testing.

This worked with <atomic> before <stdatomic.h> as well as with the order reversed?

LGTM pending testing answer. This seems simpler than implementing http://wg21.link/p0943 outright, though that would still be the best solution IMO.

libcxx/include/atomic
558 ↗(On Diff #145817)

I checked and C guarantees that this is a macro :)

In D45470#1092260, @jfb wrote:

Here is another approach that should emit an error only when mixing headers
causes compilation problems.

Have no ideas how to test the change. -verify doesn't work with fatal errors
and libcxx doesn't use FileCheck. Performed only manual testing.

This worked with <atomic> before <stdatomic.h> as well as with the order reversed?

#include <stdatomic.h>
#include <atomic>

fatal error: too many errors emitted, stopping now [-ferror-limit=]

#include <atomic>
#include <stdatomic.h>

no errors

So when <atomic> is after <stdatomic.h>, I add one more error to existing. When <atomic> is before <stdatomic.h>, there are no errors and I don't add anything.

jfb accepted this revision.May 12 2018, 12:02 PM
This revision is now accepted and ready to land.May 12 2018, 12:02 PM
This revision was automatically updated to reflect the committed changes.

Thanks for review.