[PATCH] D88434: [clangd] Improve PopulateSwitch tweak to work on non-empty switches

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Very nice, thank you!
A few nits about comments and asserts.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:112
 
-  // If there aren't any enumerators, there's nothing to insert.
-  if (EnumD->enumerator_begin() == EnumD->enumerator_end())
-    return false;
+  // Iterate through switch cases and enumerators at the same time, so we can
+  // exit early if we have more cases than enumerators.
----------------
Somewhere we should have a high-level comment about what we're doing here:

We trigger if there are fewer cases than enum values (and no case covers multiple values).
This guarantees we'll have at least one case to insert.
We don't yet determine what the cases are, as that means evaluating expressions.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:119
+       CaseList && I != E; CaseList = CaseList->getNextSwitchCase(), I++) {
+    // Switch has a default case.
+    if (isa<DefaultStmt>(CaseList))
----------------
Code is obvious here, say why instead?

"Default likely intends to cover cases we'd insert."?

(Sometimes this is still interesting in that case, so we could consider downgrading from "fix" to "refactoring" or so in this case. But it makes sense to be conservative for now)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:123
+
+    const CaseStmt *CS = dyn_cast<CaseStmt>(CaseList);
+    if (!CS)
----------------
nit: just `cast<CaseStmt>` and don't check for null (it'll automatically assert). There are no other options.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:127
+
+    // Case expression is a GNU range.
+    if (CS->caseStmtIsGNURange())
----------------
, so our counting argument doesn't work.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:131
+
+    // Case expression is not a constant expression or is value-dependent.
+    const ConstantExpr *CE = dyn_cast<ConstantExpr>(CS->getLHS());
----------------
"We may not be able to work out which cases are covered"?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:152
+    const CaseStmt *CS = dyn_cast<CaseStmt>(CaseList);
+    if (!CS || CS->caseStmtIsGNURange())
+      continue;
----------------
this can be an assert (and previous line a cast) - apply() never gets called unless prepare() returns true


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:156
+    const ConstantExpr *CE = dyn_cast_or_null<ConstantExpr>(CS->getLHS());
+    if (!CE || CE->isValueDependent())
+      continue;
----------------
similarly here


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:183
+
+  if (Text.empty())
+    return error("Populate switch: no enumerators to insert.");
----------------
this is also an assert, right? I don't think we can get here.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2838
+      {
+          // All enumerators already in switch (multiple, unscoped)
+          Function,
----------------
is there an interesting difference between the single/multiple case that wouldn't be covered by just the multiple case?
(and below)


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2838
+      {
+          // All enumerators already in switch (multiple, unscoped)
+          Function,
----------------
sammccall wrote:
> is there an interesting difference between the single/multiple case that wouldn't be covered by just the multiple case?
> (and below)
can we add a case with `default`?
(And optionally template/gnu range, though those are pretty rare)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88434