[PATCH] D66732: [Clangd] ExtractFunction Added checks for broken control flow

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:433
+    // inside Extraction Zone.
+    void incrementLoopSwitchCounters(Stmt *S) {
+      if (CurrentLocation != ZoneRelative::Inside)
----------------
rather than writing this twice, take an `int Increment` parameter?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:480
+      if (CurrentLocation == ZoneRelative::Inside &&
+          !(CurNumberOfLoops + CurNumberOfSwitch))
+        Info.BrokenControlFlow = true;
----------------
`(NumberOfLoops > 0 || NumberOfSwitch = 0)`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:497
+    // stack inside Extraction Zone. Used to check for broken control flow.
+    unsigned CurNumberOfLoops = 0;
+    unsigned CurNumberOfSwitch = 0;
----------------
NestedLoops would be clearer I think


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:523
   EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted"));
+  // We should be able to extract break/continue with a parent loop/switch.
+  EXPECT_THAT(apply(" [[for(;;) if(1) break;]] "), HasSubstr("extracted"));
----------------
please add these to a new test case e.g. `TEST_F(ExtractFunctionTest, ControlFlow)` to avoid each getting too long


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66732/new/

https://reviews.llvm.org/D66732