[Refactoring] SR-5743 Try To Force Try Refactor implementation #12128
Conversation
| struct TryExprCounter: public SourceEntityWalker { | ||
| unsigned int Count = 0; | ||
| bool walkToExprPre(Expr *E) { | ||
| if (auto *FE = dyn_cast<TryExpr>(E)) { |
There was a problem hiding this comment.
Here we don't need FE; so isa<TryExpr>(E) is sufficient.
| SM, | ||
| DCStmt->getStartLoc(), | ||
| FirstNodeStartLoc); | ||
| EditConsumer.accept(SM, BeforeBodyRange, ""); |
There was a problem hiding this comment.
Maybe a method in EditorConsumer called remove(SM, CharSourceRange) is cleaner than this. Could you add that method?
| FirstNodeStartLoc); | ||
| EditConsumer.accept(SM, BeforeBodyRange, ""); | ||
| auto LastNodePastEndLoc = BCStmt->getElements() | ||
| .back().getEndLoc().getAdvancedLocOrInvalid(1); |
There was a problem hiding this comment.
We are not sure the last token is of length 1. Using Lexer::getLocForEndOfToken instead.
| SM, | ||
| LastNodePastEndLoc, | ||
| CatchRBraceLoc); | ||
| EditConsumer.accept(SM, AfterBodyRange, ""); |
There was a problem hiding this comment.
Use the newly added remove function here too.
|
@Kacper20 thinking more about this, I'm little concerned that this refactoring is a transition from a more recommended language usage to a less recommended one. Personally, I'm not sure this is what we want to implicitly encourage. What do you think? |
|
Yeah, I think the same. Wasn't convinced by this but wanted to make one to learn more about the refactorings. Closing this and moving JIRA ticket to Resolved, then. I will add |
Opposite refactor to this one.
Behavior is as follows:
do/catchstatement — statement is removed, code that is contained indoblock is untouched (except of added exclamation totry)tryexpr that isn't nested in anydo/catch— exclamation is addeddo/catchstatement — exclamation is addedI was thinking a lot about doing some more work in the last case. The alternative could be to take all of the nodes that are contained in
BraceStmt, find the node that contains (or is) the selectedTryExprand extract it along with all the preceding nodes out of thedo/catchstatement. This is easy to implement, but I'm not sure whether it'll not be confusing to the user. What do you think?Resolves SR-5743.
cc @nkcsgexi @harlanhaskins