Skip to content

Semantics attribute definition file #28037

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 7 commits into from
Nov 20, 2019
Merged

Semantics attribute definition file #28037

merged 7 commits into from
Nov 20, 2019

Conversation

zoecarver
Copy link
Contributor

This patch creates a definition file for all non-specialized semantics attributes. Its the first in what will probably be three patches. The goal is to enforce that all semantics attributes are constant string literals that are defined in one place which will reduce the possibility for error and create a centralized place for all semantics attributes to live.

The next patch will add a category so that we can replace code such as the following:

hasSemanticsAttrThatStartsWith("array.")

with something like

hasSemanticsAttrCategory(ARRAY)

Last, it might be nice to have some way to enforce that hasSemanticsAttr is passed a defined attribute.

Additionally, here are some semantics that I found references to in the compiler but could not find references to in the standard library:

verify.ownership.sil.never
verify.ownership.sil.never
self_no_escaping_closure
pair_no_escaping_closure
inline_late

@zoecarver
Copy link
Contributor Author

cc @gottesmm

owenv
owenv reviewed Nov 2, 2019
View reviewed changes
Copy link
Contributor

@owenv owenv left a comment

Choose a reason for hiding this comment

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

I think this is a reasonable refactor, but someone more familiar with @_semantics should take a look as well. In the meantime I just have a few small comments.

include/swift/Basic/Semantics.h Outdated
#include "llvm/ADT/StringRef.h"

namespace swift {
#define SEMA_ATTR(NAME, C_STR) constexpr static const StringLiteral NAME = #C_STR;
Copy link
Contributor

Choose a reason for hiding this comment

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

These should probably go in their own namespace so they don't clutter the top level swift namespace, similar to how diagnostic definitions are in swift::diag::

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not opposed to this but let's see what others think.

gottesmm
gottesmm reviewed Nov 3, 2019
View reviewed changes
Copy link
Contributor

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

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

I am also tending to think we should put this into a namespace like semantics or perhaps strings? Not sure. @atrick @jrose-apple any ideas?

@zoecarver
Copy link
Contributor Author

I added a semantics namespace. Now that it's there, it will be easy to change or remove with a find/replace all.

@zoecarver
Address review comments
2ca448b 
* add namespace
* fix block comment style
* SEMA_ATTR -> SEMANTICS_ATTR
* error when SEMANTICS_ATTR isn't defined
@zoecarver
fixup! Address review comments
a800c9e
@slavapestov
Copy link
Contributor

@swift-ci Please test

@slavapestov slavapestov self-assigned this Nov 19, 2019
@slavapestov
Copy link
Contributor

This looks good to me. Since nobody else has commented on this in 2 weeks, I think we can merge it if the tests pass.

@slavapestov
Copy link
Contributor

If you want, you can remove the semantics not used by the stdlib in a subsequent PR.

@slavapestov slavapestov mentioned this pull request Nov 19, 2019
Closed
@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - a800c9e

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - a800c9e

@slavapestov
Copy link
Contributor

Looks like a few test failures snuck in since this was last tested:

01:08:48 Failing Tests (9):
01:08:48     Swift(linux-x86_64) :: SILOptimizer/string_switch.swift
01:08:48     Swift(linux-x86_64) :: SILOptimizer/no_size_specialization.swift
01:08:48     Swift(linux-x86_64) :: SILOptimizer/pound_assert.swift
01:08:48     Swift(linux-x86_64) :: SILOptimizer/diagnostic_constant_propagation.swift
01:08:48     Swift(linux-x86_64) :: SILOptimizer/constant_evaluator_test.sil
01:08:48     Swift(linux-x86_64) :: SILOptimizer/concat_string_literals.64.swift
01:08:48     Swift(linux-x86_64) :: SILOptimizer/constant_evaluable_subset_test.swift
01:08:48     Swift(linux-x86_64) :: SILOptimizer/constant_propagation.sil
01:08:48     Swift(linux-x86_64) :: SILOptimizer/OSLogPrototypeCompileTest.sil

Hopefully it's a simple typo somewhere.

@zoecarver
Copy link
Contributor Author

Ha! I'm surprised those are the only ones that failed. I accidentally initialized all the semantics as foo = "\"abc\"" not foo = "abc". Should be fixed now.

@slavapestov
Copy link
Contributor

@swift-ci Please test

1 similar comment
@slavapestov
Copy link
Contributor

@swift-ci Please test

@zoecarver
Copy link
Contributor Author

@slavapestov looks like the bots maybe didn't get triggered?

@theblixguy
Copy link
Collaborator

Yeah, there was an outage, which is now fixed.

@swift-ci please test

@slavapestov slavapestov merged commit 291364b into swiftlang:master Nov 20, 2019
@zoecarver
Copy link
Contributor Author

Thank you @slavapestov!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gottesmm gottesmm gottesmm left review comments

@owenv owenv owenv left review comments

Assignees

@slavapestov slavapestov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zoecarver @slavapestov @swift-ci @theblixguy @gottesmm @owenv