thakis marked an inline comment as done.
thakis added inline comments.
================
Comment at: clang/test/Parser/objc-static-assert.mm:1
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+
----------------
aaron.ballman wrote:
> erik.pilkington wrote:
> > 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.
> > Yeah, we shouldn't be treating static_assert as a keyword in C++98 or C, I
> > think. It would break code.
>
> That depends on how we implemented the feature (we could parse the token as
> an identifier and check the spelling in situations where static_assert() can
> grammatically appear, for instance). I do have a hunch that this should be
> possible to support, though it's nontrivial and I don't expect @thakis to do
> it as part of this feature.
>
> > 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.
>
> Yeah, as much as I didn't like the idea at first blush, I'm starting to think
> it's the best way forward. I don't want to ever explain why
> `__has_feature(objc_static_assert)` return true with `-std=c++98` and yet
> `static_assert(true, "");` fails to compile while `_Static_assert(true, "")`
> succeeds. That's inexplicable behavior, IMO. Having two feature test macros
> alleviates that concern.
>
> > 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.
>
> That seems reasonable to me.
One last try to make a case for this patch as-is:
> 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).
You can if you assume C++11, which most people probably do. And people who
don't either use Objective-C (without ++) where _Static_assert always works, or
use something like
```
#if __cplusplus >= 201103L
#define STATIC_ASSERT(x) static_assert(x)
#else
#define STATIC_ASSERT(x) static_assert(x)
#endif
```
For all three cases,
```
@interface Foo {
#if __has_feature(objc_static_assert)
STATIC_ASSERT(...)
#endif
}
```
will do the right thing (with STATIC_ASSERT either being static_assert for
folks who target C++11 and never, _Static_assert() (or static_assert + include
assert.h) for Objective-C without ++, or literally STATIC_ASSERT with the macro
above for people who target c++98 and c++11.
I don't think there's anything strange about this, and it allows having just a
single feature check.
I don't feel super strongly about this, so if this still doesn't sway y'all
I'll go with two features.
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
