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.

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

[IDE] Refactoring: Expand a ternary operator into an if statement and vice-versa #12554

Merged
merged 7 commits into from Dec 21, 2017

Conversation

Projects
None yet
4 participants
@romanroibu
Contributor

romanroibu commented Oct 22, 2017

When selecting a ternary expression

let x = expression ? a : b

Suggest expanding to an if-else:

let x: Int
if expression {
    x = a
} else {
    x = b
}

And vice-versa.

Feedback is very much appreciated.

Resolves SR-5794.

@DougGregor DougGregor requested a review from nkcsgexi Oct 24, 2017

@nkcsgexi

@romanroibu Overall looks good! 👍 I've reviewed the first half for now. I think it's a generally good idea to have one commit for each refactoring action though.

lib/IDE/Refactoring.cpp Outdated
@@ -1667,6 +1667,366 @@ bool RefactoringActionConvertStringsConcatenationToInterpolation::performChange(
return false;
}
struct ExpandableTernaryExprInfo {

This comment has been minimized.

@nkcsgexi

nkcsgexi Oct 26, 2017

Contributor

Can we make two sub-class of ExpandableTernaryExprInfo? Currently the logic for PatternBindingDecl and AssignExpr seems to be intertwined.

lib/IDE/Refactoring.cpp Outdated
ExpandableTernaryExprInfo Target;
if (auto D = Info.ContainedNodes[0].dyn_cast<Decl*>())
Target.setBinding(dyn_cast<PatternBindingDecl>(D));

This comment has been minimized.

@nkcsgexi

nkcsgexi Oct 26, 2017

Contributor

Once we separate the logic for PatternBindingDecl and AssignExpr, we can use constructors to pass the decl or the expr without using setX() functions.

lib/IDE/Refactoring.cpp Outdated
return true; //abort
auto NameRange = Target.getNameRange();
auto NameCharRange = Lexer::getCharSourceRangeFromSourceRange(SM, NameRange);

This comment has been minimized.

@nkcsgexi

nkcsgexi Oct 26, 2017

Contributor

can we simplify the logic by putting Lexer::getCharSourceRangeFromSourceRange into the getNameRange function.

lib/IDE/Refactoring.cpp Outdated
llvm::raw_svector_ostream OS(DeclBuffer);
std::string space(" ");
std::string newline("\n");

This comment has been minimized.

@nkcsgexi

nkcsgexi Oct 26, 2017

Contributor

We can use StringRef Space = " " here. Also, It's good practice to have consistent name conventions, i.e. to have "Space" and "NewLine" Here.

@romanroibu

This comment has been minimized.

Contributor

romanroibu commented Oct 27, 2017

Thanks for the review, @nkcsgexi ! I'll commit the fixes and get back to you ;)

@romanroibu

This comment has been minimized.

Contributor

romanroibu commented Nov 1, 2017

@nkcsgexi I've updated the PR with the changes you suggested. Let me know what else can be improved 👍

@nkcsgexi

Looks much better! Though, I've some new comments; could you take a look?

lib/IDE/Refactoring.cpp Outdated
@@ -1667,6 +1667,391 @@ bool RefactoringActionConvertStringsConcatenationToInterpolation::performChange(
return false;
}
class ExpandableTernaryExprInfo {

This comment has been minimized.

@nkcsgexi

nkcsgexi Nov 2, 2017

Contributor

could you add some doc-comments above these classes?

lib/IDE/Refactoring.cpp Outdated
public:
virtual ~ExpandableTernaryExprInfo() {}
virtual swift::IfExpr *getIf() = 0;

This comment has been minimized.

@nkcsgexi

nkcsgexi Nov 2, 2017

Contributor

We don't need swift:: here. Could you clean them?

lib/IDE/Refactoring.cpp Outdated
}
swift::Type getType() {
return swift::Type();

This comment has been minimized.

@nkcsgexi

nkcsgexi Nov 2, 2017

Contributor

return nullptr; should be sufficient here.

lib/IDE/Refactoring.cpp Outdated
if (auto Pattern = getNamePattern())
return Pattern->getType();
return swift::Type();

This comment has been minimized.

@nkcsgexi

nkcsgexi Nov 2, 2017

Contributor

return nullptr; here similarly.

lib/IDE/Refactoring.cpp Outdated
if (auto D = Info.ContainedNodes[0].dyn_cast<Decl*>()) {
auto Binding = dyn_cast<PatternBindingDecl>(D);
auto BindingInfo = new ExpandableBindingTernaryExprInfo(Binding);
return std::unique_ptr<ExpandableTernaryExprInfo>(BindingInfo);

This comment has been minimized.

@nkcsgexi

nkcsgexi Nov 2, 2017

Contributor

can we replace the above two statements with return llvm::make_unique<ExpandableTernaryExprInfo>(Binding);? In this way, we don't have a single point where a pointer is exposed.

lib/IDE/Refactoring.cpp Outdated
return nullptr;
if (auto D = Info.ContainedNodes[0].dyn_cast<Decl*>()) {
auto Binding = dyn_cast<PatternBindingDecl>(D);

This comment has been minimized.

@nkcsgexi

nkcsgexi Nov 2, 2017

Contributor

Check if Binding is nullptr here and only create ExpandableBindingTernaryExprInfo when it is not. Therefore, we don't need to check whether Binding is null inside ExpandableBindingTernaryExprInfo.

lib/IDE/Refactoring.cpp Outdated
if (auto E = Info.ContainedNodes[0].dyn_cast<Expr*>()) {
auto Assign = dyn_cast<AssignExpr>(E);
auto AssignInfo = new ExpandableAssignTernaryExprInfo(Assign);

This comment has been minimized.

@nkcsgexi

nkcsgexi Nov 2, 2017

Contributor

Similarly, check if Assign is nullptr before creating ExpandableAssignTernaryExprInfo.

This comment has been minimized.

@nkcsgexi

nkcsgexi Nov 2, 2017

Contributor

e.g.

if (auto Assign = dyn_cast<AssignExpr>(E)) {
  return llvm::make_unique<>(...)...;
}
@romanroibu

This comment has been minimized.

Contributor

romanroibu commented Nov 3, 2017

I've fixed the issues you pointed out 👍

@CodaFi

This comment has been minimized.

Collaborator

CodaFi commented Nov 3, 2017

Before this gets merged, could you squash this patch down into maybe two or three larger commits that encompass each feature that got added here?

romanroibu added some commits Nov 4, 2017

@romanroibu

This comment has been minimized.

Contributor

romanroibu commented Nov 9, 2017

Hey @nkcsgexi, I've addressed your latest comments, and also squashed the changes into 2 commits. Let me know if there's anything else that needs to be improved. Thanks!

@harlanhaskins

This comment has been minimized.

Collaborator

harlanhaskins commented Dec 4, 2017

This patch looks good to me as-is! Thanks for doing this! I just wanna get one more approval from @nkcsgexi and get one last CI pass.

@swift-ci please smoke test

@romanroibu

This comment has been minimized.

Contributor

romanroibu commented Dec 8, 2017

@nkcsgexi I've fixed one issue with the tests, could you please trigger them again?

@CodaFi

This comment has been minimized.

Collaborator

CodaFi commented Dec 8, 2017

@swift-ci please smoke test

@romanroibu

This comment has been minimized.

Contributor

romanroibu commented Dec 9, 2017

I think all the issues should be solved, could you run the tests again, please?

@CodaFi

This comment has been minimized.

Collaborator

CodaFi commented Dec 16, 2017

@swift-ci please smoke test

@nkcsgexi

This comment has been minimized.

Contributor

nkcsgexi commented Dec 18, 2017

@swift-ci please smoke test

@nkcsgexi

This comment has been minimized.

Contributor

nkcsgexi commented Dec 18, 2017

@swift-ci please smoke test

@romanroibu

This comment has been minimized.

Contributor

romanroibu commented Dec 18, 2017

@nkcsgexi hmm, seems like it's failing because of the indentation. I thought the indentation is updated after the refactoring is applied, no? Should I just remove the indentation from the .expected files?

@nkcsgexi

This comment has been minimized.

Contributor

nkcsgexi commented Dec 18, 2017

@romanroibu we do indent them when users are invoking refactorings through IDE. However, indentation is another operation orchestrated by source editor. In here, the expected change will only include changes from the refactoring, which doesn't include indentation : )

@nkcsgexi

This comment has been minimized.

Contributor

nkcsgexi commented Dec 19, 2017

@swift-ci please smoke test

@nkcsgexi nkcsgexi merged commit b2d122c into apple:master Dec 21, 2017

2 checks passed

Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details
@nkcsgexi

This comment has been minimized.

Contributor

nkcsgexi commented Dec 21, 2017

@romanroibu Merged! Thank you for the awesome addition to Swift refactoring!

@romanroibu

This comment has been minimized.

Contributor

romanroibu commented Dec 21, 2017

Thank you for all the help! :)

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