Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

De-underscore @frozen, apply it to structs #24185

Merged
merged 3 commits into from
May 31, 2019
Merged

De-underscore @frozen, apply it to structs #24185

merged 3 commits into from
May 31, 2019

Conversation

airspeedswift
Copy link
Member

@airspeedswift airspeedswift commented Apr 20, 2019 •
edited
Loading

Implementation for SE-260

This leaves classes and vars with @_fixed_layout for now. It also seems like leaving the compiler using the term "fixed layout" internally makes sense.

I've added some tests for the old names, but migrated most of the tests to the new. Not sure if more tests of the old names are merited.

IHaveNoIdeaWhatImDoing.gif, sorry if this doesn't match some conventions.

@airspeedswift
Copy link
Member Author

Oops, can you make a PR a draft retroactively?

@airspeedswift airspeedswift changed the title De-underscore @frozen, apply it to structs [DNM] De-underscore @frozen, apply it to structs Apr 20, 2019
@airspeedswift
Copy link
Member Author

@swift-ci please test

@swift-ci

This comment has been minimized.

Sign in to view
@swift-ci

This comment has been minimized.

Sign in to view
@airspeedswift
Copy link
Member Author

@swift-ci please test

@swift-ci

This comment has been minimized.

Sign in to view
@swift-ci

This comment has been minimized.

Sign in to view
slavapestov
slavapestov approved these changes Apr 20, 2019
View reviewed changes
include/swift/AST/DiagnosticsSema.def
@@ -1354,8 +1354,8 @@ WARNING(pattern_type_not_usable_from_inline_warn,none,
"%select{%select{variable|constant}0|property}1 "
"should be '@usableFromInline' or public",
(bool, bool))
ERROR(pattern_type_not_usable_from_inline_fixed_layout,none,
"type referenced from a stored property in a '@_fixed_layout' struct must "
ERROR(pattern_type_not_usable_from_inline_frozen,none,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we use this diagnostic for classes too? If so it needs a %choice

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bet we do, but it also says "struct" in the message, so…

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it was always struct-specific even if it applies to both. I'll happily correct it but I'd like to do that in a follow-on PR than lump it in here.

@airspeedswift airspeedswift force-pushed the eight-thousand-salad-plates branch from ce3d92f to fadbdb2 Compare April 22, 2019 04:19
jrose-apple
jrose-apple reviewed Apr 22, 2019
View reviewed changes
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Thanks, Ben!

lib/Sema/TypeCheckAttr.cpp
@@ -1932,6 +1932,11 @@ void AttributeChecker::visitSpecializeAttr(SpecializeAttr *attr) {
}

void AttributeChecker::visitFixedLayoutAttr(FixedLayoutAttr *attr) {
if (isa<StructDecl>(D)) {
TC.diagnose(attr->getLocation(), diag::fixed_layout_struct)
.fixItReplace(attr->getRange(), "@frozen");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be getRangeWithAt (or that you should omit the @ from your replacement).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm missing something, this looks right as-is to me:

'@frozen' attribute is now used for fixed-layout structs
@_fixed_layout
^~~~~~~~~~~~~~
@frozen

Copy link
Contributor

@jrose-apple jrose-apple May 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you weren't seeing

'@frozen' attribute is now used for fixed-layout structs
@_fixed_layout
^~~~~~~~~~~~~~
 @frozen

? Which is a different thing. You can test this: expected-warning {{'@frozen' attribute is now used for fixed-layout structs}} {{1-13=@frozen}}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A @_fixed_layout public struct FixedDeprecationWarning {} // expected-warning {{'@frozen' attribute is now used for fixed-layout structs}} {{1-15=@frozen}} test passes.

lib/Sema/TypeCheckAttr.cpp
auto *ED = cast<EnumDecl>(D);
if (auto *ED = dyn_cast<EnumDecl>(D)) {
if (!ED->getModuleContext()->isResilient()) {
diagnoseAndRemoveAttr(attr, diag::enum_frozen_nonresilient, attr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check probably applies to structs as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not resolved.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar thing, this is the existing behavior, so I'd suggest landing and then a follow-up PR can make things more consistent across the two.

@airspeedswift airspeedswift mentioned this pull request May 7, 2019
Merged
@airspeedswift airspeedswift force-pushed the eight-thousand-salad-plates branch from fadbdb2 to 41cb6bd Compare May 24, 2019 18:27
@airspeedswift
Copy link
Member Author

@swift-ci please test

@airspeedswift airspeedswift changed the title [DNM] De-underscore @frozen, apply it to structs De-underscore @frozen, apply it to structs May 26, 2019
jrose-apple
jrose-apple reviewed May 28, 2019
View reviewed changes
include/swift/AST/DiagnosticsSema.def
@@ -4136,6 +4136,15 @@ ERROR(fixed_layout_attr_on_internal_type,
"%select{private|fileprivate|internal|%error|%error}1",
(DeclName, AccessLevel))

WARNING(fixed_layout_struct,
none, "'@frozen' attribute is now used for fixed-layout structs", ())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: please pick either '@frozen' or 'frozen' attribute (both here and below); having both is redundant.

@airspeedswift airspeedswift merged commit e9d4687 into swiftlang:master May 31, 2019
airspeedswift added a commit to airspeedswift/swift that referenced this pull request May 31, 2019
@airspeedswift
De-underscore @Frozen, apply it to structs (swiftlang#24185)
ff2bbb2 
* De-underscore @Frozen for enums

* Add @Frozen for structs, deprecate @_fixed_layout for them

* Switch usage from _fixed_layout to frozen
@airspeedswift airspeedswift mentioned this pull request May 31, 2019
Merged
airspeedswift added a commit that referenced this pull request May 31, 2019
@airspeedswift
De-underscore @Frozen, apply it to structs (#24185) (#25162)
0660241 
* De-underscore @Frozen for enums

* Add @Frozen for structs, deprecate @_fixed_layout for them

* Switch usage from _fixed_layout to frozen
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrose-apple jrose-apple jrose-apple left review comments

@slavapestov slavapestov slavapestov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@airspeedswift @swift-ci @slavapestov @jrose-apple