Skip to content

Conversation

@Regno
Copy link
Contributor

@Regno Regno commented Feb 2, 2020

Implement action to convert if statements from:

var e = E.case1
if e == .case1 {}
else if e == .case2 {}
else {}

to

  var e = E.case1
  switch e {
  case .case1: break
  case .case2: break
  default: break
  }

Resolves SR-5740.

P.S. I can't reopen previous PR](#26719) because of force push to branch after rebase. I remade refactoring action to resolve comments from previous PR.

@nathawes please take a look to new implementation.

Copy link
Contributor

@nathawes nathawes left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for all the improvements. Just left one suggestion to improve the tests, but I'm happy to merge this without it too.


// RUN: rm -rf %t.result && mkdir -p %t.result

// RUN: %refactor -convert-to-switch-stmt -source-filename %s -pos=9:3 -end-pos=16:4 > %t.result/L9-3.swift
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to type check the output file for each of these to make sure the code we're generating is valid and continues to type check as the compiler changes. You can do that be adding an extra RUN line like the below:

// RUN: %target-swift-frontend-typecheck %t.result/L9-3.swift

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added for each test case.

@nathawes
Copy link
Contributor

nathawes commented Feb 8, 2020

@swift-ci please test

@nathawes
Copy link
Contributor

nathawes commented Feb 8, 2020

@swift-ci Please Sourcekit Stress test

@swift-ci
Copy link
Contributor

swift-ci commented Feb 8, 2020

Build failed
Swift Test OS X Platform
Git Sha - 904bd0b

@nathawes
Copy link
Contributor

nathawes commented Feb 8, 2020

Looks like a few of the existing tests need updating to allow for the new refactoring kind being in the list:

/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/test/refactoring/RefactoringKind/basic.swift:431:30: error: CHECK-EXTRCT-METHOD-NEXT: is not on the line after the previous match
// CHECK-EXTRCT-METHOD-NEXT: Action ends
                             ^
<stdin>:4:1: note: 'next' match was here
Action ends
^
<stdin>:2:15: note: previous match ended here
Extract Method
              ^
<stdin>:3:1: note: non-matching line after previous match is here
Convert To Switch Statement

@Regno
Copy link
Contributor Author

Regno commented Feb 8, 2020

Tests are failed at cases like this:

if a {
    return
  } else {
    return
  }

I think there are no sense to convert if statement with single bool condition to switch statement. So I just skip such kind of if at isApplicable function. Now all tests should be passed.

@nathawes
Copy link
Contributor

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 904bd0b

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 904bd0b

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants