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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
