Skip to content

[AST] Eliminate IfConfigStmt #9413

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 5 commits into from
Jun 16, 2017
Merged

[AST] Eliminate IfConfigStmt #9413

merged 5 commits into from
Jun 16, 2017

Conversation

@rintaro
Copy link
Member

@rintaro rintaro commented May 9, 2017 •
edited
Loading

NOTE: This conflicts with #7955 , I will rebase this PR when it's merged. landed

Resolves: SR-4426

#if in statements position doesn't have to be Stmt.
Modeling it as IfConfigDecl simplify the code and AST.

  • Make IfConfigDecl be able to hold ASTNode elements
  • Parse #if as IfConfigDecl
  • Stop enclosing toplevel #if into TopLevelCodeDecl.
  • New methods on IfConfigDecl for retrieving active elements as Decls. This replaces IfConfigDecl::getActiveMembers().
  • Eliminate IfConfigStmt
#if true
  #if true
  func foo() {}
  #endif
#else
  func bar() {}
#endif

is now dumped as:

(source_file
  (if_config_decl
    #if: active
      (boolean_literal_expr type='<null>' value=true)
      (elements
        (if_config_decl
          #if: active
            (boolean_literal_expr type='<null>' value=true)
            (elements
              (func_decl "foo()" ...)))
        (func_decl "foo()" ...))
    #else:
      (elements
        (func_decl "bar()" ...)))
  (func_decl "foo()" ...))

@rintaro
Copy link
Member Author

rintaro commented May 9, 2017

@slavapestov @jrose-apple
What do you think?

@rintaro
Copy link
Member Author

rintaro commented May 9, 2017

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor

@swift-ci Please test source compatibility

rintaro
rintaro commented May 9, 2017
View reviewed changes
include/swift/AST/Decl.h Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

These new methods looks overkill since they only have one client.

include/swift/AST/Stmt.h Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Accidental addition.

lib/AST/ASTDumper.cpp Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Should add : after #else.

@rintaro rintaro force-pushed the ast-eliminate-ifconfigstmt branch 3 times, most recently from 10c93c4 to 1266ada Compare May 9, 2017 13:56
@rintaro
Copy link
Member Author

rintaro commented May 9, 2017

@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented May 9, 2017

@swift-ci Please test source compatibility

jrose-apple
jrose-apple reviewed May 9, 2017
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.

Can you add a test that shows that we still reject statements within #if within types, and at the top level in -parse-as-library code?

lib/AST/ASTPrinter.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you print something here, even a comment, so that people will know it's not a bug? Or, well, that it's a known bug?

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 this is just a unimplemented feature.
For now, added a commit for printing /* condition */ here.

@rintaro
Copy link
Member Author

rintaro commented May 9, 2017

@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented May 9, 2017

Compatibility test result
https://ci.swift.org/view/Pull%20Request/job/swift-PR-source-compat-suite/56/

@rintaro rintaro mentioned this pull request May 10, 2017
Merged
@CodaFi
Copy link
Contributor

CodaFi commented May 15, 2017

@rintaro Is there anything blocking you from merging this now?

@rintaro
Copy link
Member Author

rintaro commented May 16, 2017 •
edited
Loading

@CodaFi
Although I'm OK to merge this prior to #7955, I prefer to land #7955 first.
Also, this PR hasn't been explicitly approved by anyone yet.

@jrose-apple
Copy link
Contributor

I didn't see any problems but I wasn't looking too closely. Slava may also still have opinions.

rintaro added 4 commits May 16, 2017 12:09
@rintaro
[Parse] Don't Hoist IfConfig{Decl,Stmt} out of IfConfig{Decl,Stmt}
f1b0285
@rintaro
[AST] Eliminate IfConfigStmt
6fa8415 
Resolves: https://bugs.swift.org/browse/SR-4426

* Make IfConfigDecl be able to hold ASTNodes
* Parse #if as IfConfigDecl
* Stop enclosing toplevel #if into TopLevelCodeDecl.
* Eliminate IfConfigStmt
@rintaro
[ASTScope] Eliminate isRealTopLevelCodeDecl function
de7f2f7 
Now that, all TopLevelCodeDecl is real. Just don't need this.
@rintaro
[ASTPrinter] Tweak #if print
2ad22a8 
* Removed extra newlines.
* Print '/* condition */' for condition part.
@rintaro rintaro force-pushed the ast-eliminate-ifconfigstmt branch from 26629d8 to 2ad22a8 Compare May 16, 2017 03:29
@rintaro
Copy link
Member Author

rintaro commented May 16, 2017

@swift-ci Please smoke test

@rintaro rintaro requested a review from slavapestov May 16, 2017 03:34
@rintaro
[Parse] Cleanup #if directive parsing
c8d717e 
Now that, IfConfigDecl holds ASTNode regardless statements position or
declarations postion. We can construct it in single method.
@rintaro rintaro force-pushed the ast-eliminate-ifconfigstmt branch from 552dea0 to c8d717e Compare May 16, 2017 11:53
@rintaro
Copy link
Member Author

rintaro commented May 16, 2017

@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented May 19, 2017

@slavapestov Could you review this?

@CodaFi
Copy link
Contributor

CodaFi commented May 30, 2017

Just to get the validation tests in

@swift-ci please test

@rintaro
Copy link
Member Author

rintaro commented May 31, 2017

@slavapestov Ping
Any change to merge this?

@slavapestov slavapestov merged commit 732b215 into swiftlang:master Jun 16, 2017
@rintaro
Copy link
Member Author

rintaro commented Jun 17, 2017

Thanks!

@rintaro rintaro deleted the ast-eliminate-ifconfigstmt branch June 17, 2017 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slavapestov slavapestov Awaiting requested review from slavapestov

1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

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

@rintaro @slavapestov @CodaFi @jrose-apple