Skip to content

Cleanup attr.def #16031

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
Merged
merged 1 commit into from
Apr 25, 2018
Merged

Cleanup attr.def #16031

merged 1 commit into from
Apr 25, 2018

Conversation

davezarzycki
Copy link
Contributor

@davezarzycki davezarzycki commented Apr 19, 2018 •
edited
Loading

This builds on #16014, so please ignore the first commit.

  1. Formalize OnAccessor. A hack used to alias this to OnFunc.
  2. New aggregates: OnNominalType, OnGenericType, OnAbstractFunction.
  3. Consistent and self-documented (albeit custom) style in Attr.def to ease code review/audits.

@rjmccall and @slavapestov – Review questions:

  1. Now that OnAccessor is formalized, should it be removed from any attributes?
  2. Now that OnAccessor is formalized, should OnFunc be removed from any attributes?
  3. Now that new aggregate "OnXYZ" options exist, it seems like there might be some oversights in the .def file. For example, it is intentional that inline doesn't work on destructors? Also, why doesn't _alignment include OnClass? There might be more oversights. Feedback would be appreciated.
  4. The following seems to be a common attribute requirement: OnAbstractFunction & ~OnDestructor. Should it be formalized? Perhaps OnNonDestructorFunctions?

@jrose-apple
Copy link
Contributor

Classes can't be realigned today. Destructors…could be inlinable, I guess, but that would only ever come into play if you knew a class instance didn't escape, which seems super rare across module boundaries (since it's valid for an initializer to escape self).

@slavapestov
Copy link
Contributor

@inline(never) and @inline(always) don't work on destructors because I don't think there's much value in this. The stdlib does define a few @inlinable destructors though.

I'm not a fan of the idea of OnNonDestructorFunctions. I'd rather we spell it out explicitly.

@davezarzycki
Copy link
Contributor Author

Hi @slavapestov – The value in OnNonDestructorFunctions is future proofing. It makes the design intent clear so that a future (and perhaps less informed) Decl node designer knows what to do. Does that not appeal to you? If yes, then why?

As for inlining in general, do we no longer care about module private classes? For example, a private tree-like data structure where the destructor is trivially inlinable?

@slavapestov
Copy link
Contributor

@davezarzycki I'm not sure I understand your question about inlining.

@davidungar
Copy link
Contributor

@davezarzycki I'm not sure why I'm a reviewer on this PR. It doesn't seem to be code I've been into. What sort of input from me would be helpful?

@davezarzycki davezarzycki removed the request for review from davidungar April 20, 2018 02:36
@davezarzycki
Copy link
Contributor Author

Hi @slavapestov, @rjmccall, and @jrose-apple,

Just to be clear here, I want to know whether the new "OnXYZ" flags in this pull request expose any clean up opportunities or oversights in the existing .def file. If it doesn't, then I'd like to seek approval for this pull request.

I'm primarily interested in questions 1 and 2 at the top of this pull request: does the formalization of OnAccessor exposes any clean up opportunities? Any feedback would be appreciated.

I'm less concerned about questions 3 and 4. I just wanted to double check with people that there weren't any oversights with the existing "OnXYZ" flags while I was cleaning up the file.

Finally, I do think that aggregate "OnXYZ" flags are incredibly valuable, especially for people less familiar with the code, or people trying to extend the code. For example, OnNominalType is both more clear and more future proof than itemizing the current list of nominal types over multiple attributes. Toward that end, if any other useful aggregate flag combinations exist, I'd be happy to look into adding them. I suggested "on non-destructor functions" because it seemed like both a valid intent and a common pattern. I think other aggregates are potentially useful too.

PS –  Hi @davidungar. I misinterpreted how GitHub reports "reviewers" in their user interface. I thought somebody requested you as a reviewer in #16014 and therefore you should be added to this pull request too. Upon closer inspection, it seems that GitHub reports any commenter on pull request code as a "reviewer", which is confusing.

@davezarzycki
Copy link
Contributor Author

Ping. Any feedback/review on this pull request would be appreciated.

@davezarzycki davezarzycki requested a review from xedin April 23, 2018 14:05
rjmccall
rjmccall reviewed Apr 24, 2018
View reviewed changes
include/swift/AST/Attr.def Outdated
CONTEXTUAL_SIMPLE_DECL_ATTR(postfix, Postfix,
OnFunc | OnAccessor | OnOperator |
DeclModifier,
25)
Copy link
Contributor

Choose a reason for hiding this comment

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

infix/prefix/postfix can't ever be on accessors.

include/swift/AST/Attr.def Outdated
SIMPLE_DECL_ATTR(LLDBDebuggerFunction, LLDBDebuggerFunction,
OnFunc | OnAccessor |
UserInaccessible,
17)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this can be on accessors.

include/swift/AST/Attr.def Outdated
OnFunc, 11)

OnFunc | OnAccessor,
11)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think IBAction can be on accessors.

@davezarzycki
[AST] NFC: Attr.def clean up
7b94731 
1) Formalize "OnAccessor". A hack used to alias this to "OnFunc".
2) New aggregates: OnNominalType, OnGenericType, OnAbstractFunction.
3) Consistent and self-documented (albeit custom) style to ease code review/audits.
4) Removes a few cases of `OnAccessor` based on pull request swiftlang#16031 feedback.
@davezarzycki
Copy link
Contributor Author

davezarzycki commented Apr 24, 2018 •
edited
Loading

Hi @rjmccall – I've addressed your feedback and added tests. For whatever it may be worth, the operator attributes don't even parse in an accessor context.

@swift-ci please smoke test

@davezarzycki
Copy link
Contributor Author

PS – I'm trying to respect the tentative "commits 'require' review" goal, so if somebody could approve this, I'd appreciate it. Thanks.

@rjmccall
Copy link
Contributor

Yeah, those attributes require very specific syntax which is tied in the parser to func declarations.

@davezarzycki davezarzycki merged commit e57467a into swiftlang:master Apr 25, 2018
@davezarzycki davezarzycki deleted the cleanup_Attr.def branch April 25, 2018 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjmccall rjmccall rjmccall approved these changes

@slavapestov slavapestov Awaiting requested review from slavapestov

@xedin xedin Awaiting requested review from xedin

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

Successfully merging this pull request may close these issues.
5 participants
@davezarzycki @jrose-apple @slavapestov @davidungar @rjmccall