Page MenuHomePhabricator

[refactor] Initial outline of implementation of "extract function" refactoring
ClosedPublic

Authored by arphaman on Oct 16 2017, 4:35 PM.

Details

Summary

This patch adds an initial, skeleton outline of the "extract function" refactoring.
The extracted function doesn't capture variables / rewrite code yet, it just basically does a simple copy-paste.
The following initiation rules are specified:

  • extraction can only be done for executable code in a function/method/block. This means that you can't extract a global variable initialize into a function right now (should this be allowed though?).
  • simple literals and references are not extractable.

This patch also adds support for full source ranges to clang-refactor's test mode.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Oct 16 2017, 4:35 PM
ioeric edited edge metadata.Oct 18 2017, 5:45 AM

Code looks good in general. I see there are a bunch of major features missing; it might make sense to print a warning or document the major missing features in the high-level API.

include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
74 ↗(On Diff #119228)

Could we find a better name for this? It's not clear what the difference between this and SourceRangeSelectionRequirement is from the names.

include/clang/Tooling/Refactoring/RefactoringRuleContext.h
84 ↗(On Diff #119228)

Maybe ASTNodeSelection for clarity?

lib/Tooling/Refactoring/ASTSelection.cpp
340 ↗(On Diff #119228)

A short explanation of the for-loop would be appreciated :)

lib/Tooling/Refactoring/Extract.cpp
167 ↗(On Diff #119228)

An alternative way to get the source code is:

Lexer::getSourceText(
      CharSourceRange::getTokenRange(SM.getSpellingLoc(Start), SM.getSpellingLoc(End)),
      SM, Result.Context->getLangOpts());
tools/clang-refactor/TestSupport.cpp
106 ↗(On Diff #119228)
arphaman updated this revision to Diff 119708.Oct 20 2017, 2:12 PM
arphaman marked 4 inline comments as done.

Address review comments.

Code looks good in general. I see there are a bunch of major features missing; it might make sense to print a warning or document the major missing features in the high-level API.

I added a warning in the description of the action.

include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
75 ↗(On Diff #119708)

Renamed to CodeRangeASTSelectionRequirement

lib/Tooling/Refactoring/Extract.cpp
167 ↗(On Diff #119228)

I will need to get the rewritten source, so I've started using it in the first patch.

tools/clang-refactor/TestSupport.cpp
106 ↗(On Diff #119228)

Fixed.

ioeric accepted this revision.Oct 24 2017, 8:23 AM

Lgtm

include/clang/Basic/DiagnosticRefactoringKinds.td
24 ↗(On Diff #119711)

nit: was this newline intended?

tools/clang-refactor/TestSupport.cpp
370 ↗(On Diff #119711)

nit: curly braces in else-branch if if-branch has braces.

This revision is now accepted and ready to land.Oct 24 2017, 8:23 AM
This revision was automatically updated to reflect the committed changes.
arphaman marked an inline comment as done.
arphaman added inline comments.Oct 24 2017, 10:19 AM
include/clang/Basic/DiagnosticRefactoringKinds.td
24 ↗(On Diff #119711)

Yeah, it separates the extraction errors from the others.