This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Allow extract-to-function on regions that always return.
ClosedPublic

Authored by sammccall on Nov 21 2019, 2:14 PM.

Details

Summary

We only do a trivial check whether the region always returns - it has to end
with a return statement.

Diff Detail

Event Timeline

sammccall created this revision.Nov 21 2019, 2:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 21 2019, 2:14 PM

Build result: pass - 60231 tests passed, 0 failed and 732 were skipped.
Log files: console-log.txt, CMakeCache.txt

Build result: pass - 60231 tests passed, 0 failed and 732 were skipped.

Log files: console-log.txt, CMakeCache.txt

kadircet accepted this revision.Nov 22 2019, 2:45 AM
kadircet marked an inline comment as done.

LGTM, thanks!

clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
173

EZ.getLastRootStmt() instead of EZ.Parent->Children.back()

175

nit: braces

180

what if there's a goto in the selection :P

611

nit: early exits

This revision is now accepted and ready to land.Nov 22 2019, 2:45 AM

btw forgot to add a comment, it might be worth mentioning in change description that, this works in its current state because we don't allow extractions inside lambda expressions.

sammccall marked 2 inline comments as done.Nov 22 2019, 2:54 AM

(Thanks! Will address comments, just wanted to ask for a clarification)

clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
180

If the goto target is in the selection, that's fine. Otherwise it's broken control flow like break/continue. It doesn't seem to be detected yet indeed.

611

I'm not sure what this comment means, can you elaborate?
(I do mean "sometimes returns and sometimes doesn't". Early-exits aren't a problem, e.g. if (x) return 1; return 2; can be extracted.)

kadircet added inline comments.Nov 22 2019, 3:53 AM
clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
611

ah sorry, i was trying to imply usage of early exits in the code, i.e:

ExtractedFunc.ReturnType = EnclosingFunc.getParentASTContext().VoidTy;
if not HasReturnStmt
  return true;
if not AlwaysReturn
  return false;
.
.
.
This revision was automatically updated to reflect the committed changes.
sammccall marked 3 inline comments as done.