Page MenuHomePhabricator

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

Authored by thakis on Mar 11 2019, 10:43 AM.

Details

Summary

This adds support for static_assert() (and _Static_assert()) in @interface/@implementation ivar lists and in @interface method declarations.

It was already supported in @implementation blocks outside of the ivar lists.

The assert AST nodes are added at file scoped, matching where other (non-Objective-C) declarations at @interface / @implementation level go (cf allTUVariables).

Also add a __has_feature(objc_c_static_assert) that's true in C11 (and __has_extension(objc_c_static_assert) that's always true) and __has_feature(objc_cxx_static_assert) that's true in C++11 modeafter this patch, so it's possible to check if this is supported.

Diff Detail

Repository
rC Clang

Event Timeline

thakis created this revision.Mar 11 2019, 10:43 AM

(Since jfb added a few people who might not have seen it: This was discussed in "Objective-C++11: concerns about adding static_assert support to @interface / @implementation?" on cfe-dev)

aaron.ballman added inline comments.
clang/lib/Parse/ParseDecl.cpp
3892–3893 ↗(On Diff #190123)

Seems to be unrelated to this patch? Feel free to commit separately.

clang/test/Parser/objc-static-assert.mm
1 ↗(On Diff #190123)

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().

thakis marked 2 inline comments as done.Mar 11 2019, 1:17 PM
thakis added inline comments.
clang/lib/Parse/ParseDecl.cpp
3892–3893 ↗(On Diff #190123)

It's somewhat related in that ParseStructDeclaration() is called a few lines below where I'm adding the comment talking about ParseStructUnionBody()

clang/test/Parser/objc-static-assert.mm
1 ↗(On Diff #190123)

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.)

aaron.ballman added inline comments.Mar 11 2019, 1:35 PM
clang/test/Parser/objc-static-assert.mm
1 ↗(On Diff #190123)

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().

thakis marked an inline comment as done.

test c++98

clang/test/Parser/objc-static-assert.mm
1 ↗(On Diff #190123)

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.)

clang/test/Parser/objc-static-assert.mm
1 ↗(On Diff #190123)

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.

aaron.ballman added inline comments.
clang/test/Parser/objc-static-assert.mm
1 ↗(On Diff #190123)

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.

thakis marked an inline comment as done.Mar 11 2019, 6:04 PM
thakis added inline comments.
clang/test/Parser/objc-static-assert.mm
1 ↗(On Diff #190123)

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.

thakis edited the summary of this revision. (Show Details)

two features

Seems like this wasn't convincing, so now with two separate feature flags. I went with the shorter name, since adding "interface_" isn't just longer but also misleading, since this also works in @implementation ivar blocks now.

erik.pilkington accepted this revision.Mar 13 2019, 2:15 PM

LGTM, after one more comment in Features.def :)

clang/include/clang/Basic/Features.def
121 ↗(On Diff #190418)

We should only claim _Static_assert is a feature in languages where its actually a formal language feature (C11), you can also use EXTENSION to indicate what languages we accept it in (see the comment in the file header).

This revision is now accepted and ready to land.Mar 13 2019, 2:15 PM
thakis marked an inline comment as done.Mar 13 2019, 4:00 PM
thakis added inline comments.
clang/include/clang/Basic/Features.def
121 ↗(On Diff #190418)

Sorry, I don't follow. clang accepts _Static_assert in C89. You're saying you want __has_feature(objc_c_static_assert) to be false there? I'm guessing because -pedantic warns on it?

But if we do this, there's no way to differentiate clang-in-std=c89 mode with this patch from clang-without-this-patch, is there? Is that what you want? i.e. replace true with LangOpts.C11?

(For now, I updated the test to check the __has_feature() is true in c89 too.)

(That seems like another argument to restore the single feature to me since this working in objc is orthogonal on if the language version supports static asserts.)

clang/include/clang/Basic/Features.def
121 ↗(On Diff #190418)

Sorry, I don't follow. clang accepts _Static_assert in C89. You're saying you want __has_feature(objc_c_static_assert) to be false there? I'm guessing because -pedantic warns on it?

Yeah, __has_feature is meant to check for standard language features. __has_extension checks whether clang will accept it, regardless of what respective standard says. So _Static_assert here is a "standard" language feature in ObjC + C11 and an extension in ObjC + C99. I'm suggesting you write this like:

FEATURE(objc_c_static_assert, LangOpts.C11)
FEATURE(objc_cxx_static_assert, LangOpts.CPlusPlus11)
EXTENSION(objc_c_static_assert, true)

Just like we do now for c_static_assert in general:

FEATURE(c_static_assert, LangOpts.C11)
EXTENSION(c_static_assert, true)

That way, __has_extension(objc_c_static_assert) is always true (with a new compiler), and __has_feature only deals with standard language features. Does that make sense?

thakis edited the summary of this revision. (Show Details)

extension

Ah, got it, thanks for explaining!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2019, 7:18 AM
aaron.ballman added inline comments.Mar 14 2019, 7:22 AM
clang/test/Parser/objc-static-assert.m
29 ↗(On Diff #190534)

Given that static assertions are member declarations of a struct, should this "replay" the static asserts from the interface (including failing the assertions)?

clang/test/Parser/objc-static-assert.mm
4 ↗(On Diff #190534)

Why do we expect objc_c_static_assert to be a supported feature in ObjC++? I would have expected this as an extension, but not a feature.

thakis marked 2 inline comments as done.Mar 14 2019, 7:40 AM
thakis added inline comments.
clang/test/Parser/objc-static-assert.m
29 ↗(On Diff #190534)

This only works in the old fragile abi and is unsupported in "modern" (8 year old) objc, and it was used very rarely even back then. I don't think we need to do anything about this other than not crashing.

clang/test/Parser/objc-static-assert.mm
4 ↗(On Diff #190534)

Thanks for catching this, looks I forgot to re-run the .mm test after changing Features.def :-( Fixed now in r356154.

aaron.ballman added inline comments.Mar 14 2019, 7:58 AM
clang/test/Parser/objc-static-assert.m
29 ↗(On Diff #190534)

Fine by me!

clang/test/Parser/objc-static-assert.mm
4 ↗(On Diff #190534)

Great, thank you!

Other than those points, this LGTM as well. :-)