[PATCH] D88434: [clangd] Improve PopulateSwitch tweak to work on non-empty switches
This revision was automatically updated to reflect the committed changes.
Closed by commit rG4fb303f340e2: [clangd] Improve PopulateSwitch tweak to work
on non-empty switches (authored by tdeo, committed by sammccall).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88434/new/
https://reviews.llvm.org/D88434
Files:
clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2829,9 +2829,48 @@
"unavailable",
},
{
- // Existing enumerators in switch
+ // All enumerators already in switch (unscoped)
Function,
- R""(enum Enum {A}; ^switch ((Enum)0) {case A:break;})"",
+ R""(enum Enum {A,B}; ^switch (A) {case A:break;case B:break;})"",
+ "unavailable",
+ },
+ {
+ // All enumerators already in switch (scoped)
+ Function,
+ R""(
+enum class Enum {A,B};
+^switch (Enum::A) {case Enum::A:break;case Enum::B:break;}
+ )"",
+ "unavailable",
+ },
+ {
+ // Default case in switch
+ Function,
+ R""(
+enum class Enum {A,B};
+^switch (Enum::A) {default:break;}
+ )"",
+ "unavailable",
+ },
+ {
+ // GNU range in switch
+ Function,
+ R""(
+enum class Enum {A,B};
+^switch (Enum::A) {case Enum::A ... Enum::B:break;}
+ )"",
+ "unavailable",
+ },
+ {
+ // Value dependent case expression
+ File,
+ R""(
+enum class Enum {A,B};
+template
+void function() {
+^switch (Enum::A) {case Value:break;}
+}
+ )"",
"unavailable",
},
{
@@ -2867,9 +2906,53 @@
{
// Scoped enumeration with multiple enumerators
Function,
- R""(enum class Enum {A,B}; ^switch (Enum::A) {})"",
- R""(enum class Enum {A,B}; )""
- R""(switch (Enum::A) {case Enum::A:case Enum::B:break;})"",
+ R""(
+enum class Enum {A,B};
+^switch (Enum::A) {}
+ )"",
+ R""(
+enum class Enum {A,B};
+switch (Enum::A) {case Enum::A:case Enum::B:break;}
+ )"",
+ },
+ {
+ // Only filling in missing enumerators (unscoped)
+ Function,
+ R""(
+enum Enum {A,B,C};
+^switch (A) {case B:break;}
+ )"",
+ R""(
+enum Enum {A,B,C};
+switch (A) {case B:break;case A:case C:break;}
+ )"",
+ },
+ {
+ // Only filling in missing enumerators,
+ // even when using integer literals
+ Function,
+ R""(
+enum Enum {A,B=1,C};
+^switch (A) {case 1:break;}
+ )"",
+ R""(
+enum Enum {A,B=1,C};
+switch (A) {case 1:break;case A:case C:break;}
+ )"",
+ },
+ {
+ // Only filling in missing enumerators (scoped)
+ Function,
+ R""(
+enum class Enum {A,B,C};
+^switch (Enum::A)
+{case Enum::B:break;}
+ )"",
+ R""(
+enum class Enum {A,B,C};
+switch (Enum::A)
+{case Enum::B:break;case Enum::A:case Enum::C:break;}
+ )"",
},
{
// Scoped enumerations in namespace
Index: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
@@ -33,12 +33,15 @@
#include "AST.h"
#include "Selection.h"
#include "refactor/Tweak.h"
+#include "support/Logger.h"
#include "clang/AST/Decl.h"
#include "clang/AST/Stmt.h"
#include "clang/AST/Type.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/SmallSet.h"
+#include
#include
namespace clang {
@@ -52,18 +55,16 @@
Intent intent() const override { return Refactor; }
private:
- ASTContext *ASTCtx = nullptr;
const DeclContext *DeclCtx = nullptr;
const SwitchStmt *Switch = nullptr;
const CompoundStmt *Body = nullptr;
+ const EnumType *EnumT = nullptr;
const EnumDecl *EnumD = nullptr;
};
REGISTER_TWEAK(PopulateSwitch)
bool PopulateSwitch::prepare(const Selection ) {
- ASTCtx = >getASTContext();
-
const SelectionTree::Node *CA = Sel.ASTSelection.commonAncestor();
if (!CA)
[PATCH] D88434: [clangd] Improve PopulateSwitch tweak to work on non-empty switches
sammccall accepted this revision. sammccall added a comment. Thank you! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88434/new/ https://reviews.llvm.org/D88434 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88434: [clangd] Improve PopulateSwitch tweak to work on non-empty switches
tdeo marked 10 inline comments as done. tdeo added a comment. Thanks for the review! Please land this if there are no more issues. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88434/new/ https://reviews.llvm.org/D88434 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88434: [clangd] Improve PopulateSwitch tweak to work on non-empty switches
tdeo updated this revision to Diff 294959.
tdeo added a comment.
- Improve comments
- In apply(), assert on the conditions we established in prepare() instead of
proceeding.
- Remove redundant tests and add new ones for value-dependent and GNU range
cases.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88434/new/
https://reviews.llvm.org/D88434
Files:
clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2829,9 +2829,48 @@
"unavailable",
},
{
- // Existing enumerators in switch
+ // All enumerators already in switch (unscoped)
Function,
- R""(enum Enum {A}; ^switch ((Enum)0) {case A:break;})"",
+ R""(enum Enum {A,B}; ^switch (A) {case A:break;case B:break;})"",
+ "unavailable",
+ },
+ {
+ // All enumerators already in switch (scoped)
+ Function,
+ R""(
+enum class Enum {A,B};
+^switch (Enum::A) {case Enum::A:break;case Enum::B:break;}
+ )"",
+ "unavailable",
+ },
+ {
+ // Default case in switch
+ Function,
+ R""(
+enum class Enum {A,B};
+^switch (Enum::A) {default:break;}
+ )"",
+ "unavailable",
+ },
+ {
+ // GNU range in switch
+ Function,
+ R""(
+enum class Enum {A,B};
+^switch (Enum::A) {case Enum::A ... Enum::B:break;}
+ )"",
+ "unavailable",
+ },
+ {
+ // Value dependent case expression
+ File,
+ R""(
+enum class Enum {A,B};
+template
+void function() {
+^switch (Enum::A) {case Value:break;}
+}
+ )"",
"unavailable",
},
{
@@ -2867,9 +2906,53 @@
{
// Scoped enumeration with multiple enumerators
Function,
- R""(enum class Enum {A,B}; ^switch (Enum::A) {})"",
- R""(enum class Enum {A,B}; )""
- R""(switch (Enum::A) {case Enum::A:case Enum::B:break;})"",
+ R""(
+enum class Enum {A,B};
+^switch (Enum::A) {}
+ )"",
+ R""(
+enum class Enum {A,B};
+switch (Enum::A) {case Enum::A:case Enum::B:break;}
+ )"",
+ },
+ {
+ // Only filling in missing enumerators (unscoped)
+ Function,
+ R""(
+enum Enum {A,B,C};
+^switch (A) {case B:break;}
+ )"",
+ R""(
+enum Enum {A,B,C};
+switch (A) {case B:break;case A:case C:break;}
+ )"",
+ },
+ {
+ // Only filling in missing enumerators,
+ // even when using integer literals
+ Function,
+ R""(
+enum Enum {A,B=1,C};
+^switch (A) {case 1:break;}
+ )"",
+ R""(
+enum Enum {A,B=1,C};
+switch (A) {case 1:break;case A:case C:break;}
+ )"",
+ },
+ {
+ // Only filling in missing enumerators (scoped)
+ Function,
+ R""(
+enum class Enum {A,B,C};
+^switch (Enum::A)
+{case Enum::B:break;}
+ )"",
+ R""(
+enum class Enum {A,B,C};
+switch (Enum::A)
+{case Enum::B:break;case Enum::A:case Enum::C:break;}
+ )"",
},
{
// Scoped enumerations in namespace
Index: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
@@ -33,12 +33,15 @@
#include "AST.h"
#include "Selection.h"
#include "refactor/Tweak.h"
+#include "support/Logger.h"
#include "clang/AST/Decl.h"
#include "clang/AST/Stmt.h"
#include "clang/AST/Type.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/SmallSet.h"
+#include
#include
namespace clang {
@@ -52,18 +55,16 @@
Intent intent() const override { return Refactor; }
private:
- ASTContext *ASTCtx = nullptr;
const DeclContext *DeclCtx = nullptr;
const SwitchStmt *Switch = nullptr;
const CompoundStmt *Body = nullptr;
+ const EnumType *EnumT = nullptr;
const EnumDecl *EnumD = nullptr;
};
REGISTER_TWEAK(PopulateSwitch)
bool PopulateSwitch::prepare(const Selection ) {
- ASTCtx = >getASTContext();
-
const SelectionTree::Node *CA = Sel.ASTSelection.commonAncestor();
if (!CA)
[PATCH] D88434: [clangd] Improve PopulateSwitch tweak to work on non-empty switches
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(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(CaseList);
+if (!CS)
nit: just `cast` 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(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(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(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88434: [clangd] Improve PopulateSwitch tweak to work on non-empty switches
tdeo created this revision.
tdeo added a reviewer: sammccall.
tdeo added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman.
Herald added a project: clang.
tdeo requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
Improve the recently-added PopulateSwitch tweak to work on non-empty switches.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D88434
Files:
clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -2829,9 +2829,33 @@
"unavailable",
},
{
- // Existing enumerators in switch
+ // All enumerators already in switch (single, unscoped)
Function,
- R""(enum Enum {A}; ^switch ((Enum)0) {case A:break;})"",
+ R""(enum Enum {A}; ^switch (A) {case A:break;})"",
+ "unavailable",
+ },
+ {
+ // All enumerators already in switch (multiple, unscoped)
+ Function,
+ R""(enum Enum {A,B}; ^switch (A) {case A:break;case B:break;})"",
+ "unavailable",
+ },
+ {
+ // All enumerators already in switch (single, scoped)
+ Function,
+ R""(
+enum class Enum {A};
+^switch (Enum::A) {case Enum::A:break;}
+ )"",
+ "unavailable",
+ },
+ {
+ // All enumerators already in switch (multiple, scoped)
+ Function,
+ R""(
+enum class Enum {A,B};
+^switch (Enum::A) {case Enum::A:break;case Enum::B:break;}
+ )"",
"unavailable",
},
{
@@ -2867,9 +2891,53 @@
{
// Scoped enumeration with multiple enumerators
Function,
- R""(enum class Enum {A,B}; ^switch (Enum::A) {})"",
- R""(enum class Enum {A,B}; )""
- R""(switch (Enum::A) {case Enum::A:case Enum::B:break;})"",
+ R""(
+enum class Enum {A,B};
+^switch (Enum::A) {}
+ )"",
+ R""(
+enum class Enum {A,B};
+switch (Enum::A) {case Enum::A:case Enum::B:break;}
+ )"",
+ },
+ {
+ // Only filling in missing enumerators (unscoped)
+ Function,
+ R""(
+enum Enum {A,B,C};
+^switch (A) {case B:break;}
+ )"",
+ R""(
+enum Enum {A,B,C};
+switch (A) {case B:break;case A:case C:break;}
+ )"",
+ },
+ {
+ // Only filling in missing enumerators,
+ // even when using integer literals
+ Function,
+ R""(
+enum Enum {A,B=1,C};
+^switch (A) {case 1:break;}
+ )"",
+ R""(
+enum Enum {A,B=1,C};
+switch (A) {case 1:break;case A:case C:break;}
+ )"",
+ },
+ {
+ // Only filling in missing enumerators (scoped)
+ Function,
+ R""(
+enum class Enum {A,B,C};
+^switch (Enum::A)
+{case Enum::B:break;}
+ )"",
+ R""(
+enum class Enum {A,B,C};
+switch (Enum::A)
+{case Enum::B:break;case Enum::A:case Enum::C:break;}
+ )"",
},
{
// Scoped enumerations in namespace
Index: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
@@ -33,12 +33,14 @@
#include "AST.h"
#include "Selection.h"
#include "refactor/Tweak.h"
+#include "support/Logger.h"
#include "clang/AST/Decl.h"
#include "clang/AST/Stmt.h"
#include "clang/AST/Type.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/SmallSet.h"
#include
namespace clang {
@@ -52,18 +54,16 @@
Intent intent() const override { return Refactor; }
private:
- ASTContext *ASTCtx = nullptr;
const DeclContext *DeclCtx = nullptr;
const SwitchStmt *Switch = nullptr;
const CompoundStmt *Body = nullptr;
+ const EnumType *EnumT = nullptr;
const EnumDecl *EnumD = nullptr;
};
REGISTER_TWEAK(PopulateSwitch)
bool PopulateSwitch::prepare(const Selection ) {
- ASTCtx = >getASTContext();
-
const SelectionTree::Node *CA = Sel.ASTSelection.commonAncestor();
if (!CA)
return false;
@@ -94,11 +94,6 @@
if (!Body)
return false;
- // Since we currently always insert all enumerators, don't suggest this tweak
- // if the body is not empty.
- if (!Body->body_empty())
-
