[PATCH] D88383: [clangd] Add a tweak for filling in enumerators of a switch statement.

[Sam McCall via Phabricator via cfe-commits]

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

This is really nice!

We'd actually debated how to add such a feature, but had assumed it should be a "fix" rather than a "tweak" - I think this way is much better.

Obviously making it fill in the remaining enumerators in the general case would be great to have later. It looks like making prepare() make a cheap decision is the tricky part, but seems doable:

- bail out if there's a `default` or a gnu `...` case
- bail out if there are at least as many cases as enumerators
- bail out if any case expression is value-dependent
- otherwise we should be able to find some cases to insert

(We're subject to AST error-recovery weirdness - e.g. CaseStmt with non-constants seem to just be deleted - but probably fine to ignore all these).



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:96
+  // don't suggest this tweak if there are already cases in the switch.
+  if (Switch->getSwitchCaseList())
+    return false;
----------------
while it's uncommon, there can be stuff other than cases in a switch stmt.
Maybe we simply want to bail out if the body is nonempty?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:104
+  // Ignore implicit casts, since enums implicitly cast to integer types.
+  EnumT = Switch->getCond()
+              ->IgnoreParenImpCasts()
----------------
can we add a defensive nullcheck to getCond()?

code like `switch() {}` is the sort of thing that is often represented in AST as a null pointer.
AFAICT that doesn't happen today (we get no AST nodes at all instead) but I can see that being changed in future to improve recovery.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:124
+  const SourceManager &SM = ASTCtx->getSourceManager();
+  SourceLocation Loc = Body->getLBracLoc().getLocWithOffset(1);
+
----------------
in the case where there's already stuff in the body (just not cases), it seems like RBracLoc-1 might be safer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88383