[PATCH] D67023: Diagnose use of ATOMIC_VAR_INIT

Wed Sep 4 13:56:34 PDT 2019

aaron.ballman added a comment.

In D67023#1656453 <https://reviews.llvm.org/D67023#1656453>, @jfb wrote:

> In D67023#1654704 <https://reviews.llvm.org/D67023#1654704>, @aaron.ballman wrote:
>
> > In D67023#1654070 <https://reviews.llvm.org/D67023#1654070>, @jfb wrote:
> >
> > > I refer you to http://wg21.link/p0883
> > >  I don’t think this diagnostic should be added to clang until p0883 is fully implemented, even just for C. Otherwise we leave users with no portable way to do the right thing without diagnostic.
> >
> >
> > P0883 has not been adopted, that I can tell (it has strong support, but isn't [expected to be] a part of C++2a)?
>
>
> I think it'll make it in as an NB comment.
>
> > Also, this functionality is implemented by libc++ and libstdc++, so I'm not certain what you mean by "until p0883 is fully implemented" or why that paper would be implemented "even just for C". Are you suggesting to gate this deprecation for C functionality based on what C++ standard library implementations are doing?
>
> Yes, because atomics are already hard enough to use correctly between C and C++.

I don't see why that should preclude us from diagnosing C code. Not everyone shares C and C++ code, and C developers should be alerted to this deprecation independent of what C++ is doing.

> Once we've fixed C++ I'd like the diagnostic to be the same for both (modulo which standard it's deprecated in). I think we should add the diagnostic for C and C++ at the same time, and we should make sure there's no libc++ nor clang work required before diagnosing.

Given that C has already made this decision, it seems like the tail wagging the dog to wait for C++ to maybe adopt the same thing as part of an NB comment.

>> I'm sorry if I'm being dense here, but I am still not seeing the issue you're concerned by. C has already deprecated this feature and is planning to remove it shortly. I am diagnosing that fact in C, which seems perfectly appropriate to do regardless of what C++ is doing in this space.
>> 
>> Users have a portable way to write their code already because `ATOMIC_VAR_INIT` does not do any special magic and no implementation requires a user to use it to achieve portable atomic initialization semantics. If they get the diagnostic (which only triggers in C mode and only if the macro was defined in stdatomic.h), they should remove the use of `ATOMIC_VAR_INIT` from their code, or disable the deprecation diagnostic. If neither of those options appeals to the user, they can write something along the lines of:
>> 
>>   _Atomic(int) i = 
>>   #if defined(__cplusplus)
>>   ATOMIC_VAR_INIT(12);
>>   #else
>>   12;
>>   #endif
>> 
>> 
>> (not that I think anyone will want to write that, but strict adherence to a broken part of both standards does not seem like something we want to encourage anyway -- this is deprecated functionality, so the whole point is to discourage its use.)
> 
> Is this correct for all implementations of `_Atomic` and `std::atomic` that clang supports, including non-lock-free types? We should make sure before diagnosing.

Let me put it another way: code that does not work with `= 12` will similarly not work with `= ATOMIC_VAR_INIT(12)` because the two are identical (modulo a `ParenExpr`) after preprocessing. So yes, I believe this is "correct" for all implementations of `_Atomic` and `std::atomic` that Clang supports insomuch as it's bug-for-bug-or-lack-thereof compatible.

I feel like we're going in circles, so perhaps if I explained my problem a different way, it will resonate more. I am worried about C developers writing C code against a C compiler when the C committee has deprecated `ATOMIC_VAR_INIT` and want to diagnose this situation in C mode. What I hear you being concerned with is: what about C++ developers? I've pointed out that they have several options if they really want full portability to any compiler in C or C++ mode, but beyond that, I am not concerned about C++ developers. They have `ATOMIC_VAR_INIT` and will until such time as WG21 decides to deprecate it, at which point I would like to diagnose in C++ mode as well.

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

https://reviews.llvm.org/D67023