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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
