[PATCH] D59223: Objective-C++11: Support static_assert() in @interface/@implementation ivar lists and method declarations

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
Comment at: clang/test/Parser/objc-static-assert.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+
----------------
thakis wrote:
> aaron.ballman wrote:
> > thakis wrote:
> > > aaron.ballman wrote:
> > > > Can you try explicitly specifying C++98 as the underlying language standard mode? I feel like `_Static_assert()` will continue to work there (because we made it a language extension in all modes) but `static_assert()` may fail (because that's gated on C++11 support). If that turns out to be the case, then I think `objc_static_assert` should be more complex than expanding to `true`, or we should talk about supporting `static_assert()` in all C++ language modes like we do for `_Static_assert()`.
> > > Correct, with -std=c++98 static_assert isn't available but _Static_assert still is. If you want, I can add a test for this, but this is covered by regular c++ tests already.
> > > 
> > > I think the has_feature() should stay as-is though: Else you have no way to know if _Static_assert works in obj-c mode, and you can check if static_assert works by checkout has_feature && __cplusplus >= 201103L if you still care about c++98.
> > > 
> > > (And adding one feature each for static_assert and _Static_assert seems like overkill.)
> > > Correct, with -std=c++98 static_assert isn't available but _Static_assert still is. If you want, I can add a test for this, but this is covered by regular c++ tests already.
> > 
> > Please do (if we don't change the availability of `static_assert()`), because the test currently relies on the unwritten -std option in order to pass.
> > 
> > > I think the has_feature() should stay as-is though: Else you have no way to know if _Static_assert works in obj-c mode, and you can check if static_assert works by checkout has_feature && __cplusplus >= 201103L if you still care about c++98.
> > 
> > I don't think this is the correct approach. Testing for `static_assert()` support should not leave the user guessing at what the correct spelling is.
> > 
> > > (And adding one feature each for static_assert and _Static_assert seems like overkill.)
> > 
> > Definitely agreed there.
> > 
> > I think the correct way forward is to support `static_assert()` in all language modes like we already do for `_Static_assert()`, then `objc_static_assert` becomes useful as-is. I cannot think of a reason why we would want to allow `_Static_assert()` in C++ but not allow `static_assert()`.
> I updated the test.
> 
> Accepting `static_assert()` independent of language mode seems pretty unrelated to this patch here, so I don't want to do this.
> 
> If you don't like the current has_feature approach, I'm all ears for other approaches. The current approach allows you to detect if clang can handle static_assert in objc objects, and codebases that still care about c++98 will have a static_assert wrapping macro keyed off __cplusplus already, so that part will transparently just work as well. And codebases that are c++11 and newer are in a good position too. I think the current setup is pretty good. (But I'm happy to hear better suggestions.)
> Accepting static_assert() independent of language mode seems pretty unrelated to this patch here, so I don't want to do this.

Yeah, we shouldn't be treating `static_assert` as a keyword in C++98 or C, I think. It would break code.

> If you don't like the current has_feature approach, I'm all ears for other approaches. The current approach allows you to detect if clang can handle static_assert in objc objects, and codebases that still care about c++98 will have a static_assert wrapping macro keyed off __cplusplus already, so that part will transparently just work as well. And codebases that are c++11 and newer are in a good position too. I think the current setup is pretty good. (But I'm happy to hear better suggestions.)

This is pretty weird. This feature flag doesn't actually correspond to any feature, just the possibility of the existence of a feature (there isn't any way you could use this `__has_feature` portably without also including another `__has_feature` check). I think the most internally consistent way of doing this is to have two flags, as you mentioned above, `objc_c_static_assert` and `objc_cxx_static_assert`.

Just to keep the bikeshed going, maybe it should be spelt `objc_interface_c_static_assert` or something, to show that it doesn't control static_assert in ObjC, but static_assert in interfaces in ObjC.


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

https://reviews.llvm.org/D59223