[PATCH] D90555: [clangd] Handle duplicate enum constants in PopulateSwitch tweak

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks!
This makes sense to me, I assumed at first that the expr could be something complicated that we'd have to const-evaluate, thought clearly this already happens (and we do assume it's a ConstantExpr).



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:115
 
-  // 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.
----------------
It would still be nice to keep a high-level comment about the strategy.
(Not *this* comment of course, as this patch changes the strategy!)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:117
 
-  // 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.
-  auto I = EnumD->enumerator_begin();
-  auto E = EnumD->enumerator_end();
+  // Special case of the empty enum
+  if (EnumD->enumerator_begin() == EnumD->enumerator_end())
----------------
why is the special case needed?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:126
+
+  llvm::SmallMapVector<llvm::APSInt, bool, 32> EnumConstants;
+  for (auto *EnumConstant : EnumD->enumerators()) {
----------------
It looks as if we're now doing (via prepare + apply combined):

1. Collect all constants into a `SmallMapVector<APSInt,bool>`.
2. Iterate over cases, marking covered constants
3. Prepare returns true if there are unmarked constants
4. Collect all cases into a `SmallSet<APSInt>`
5. Iterate over the constants, inserting text for those not in the set.

This seems equivalent, but less redundant:
1. Collect all constants into a `MapVector<APSInt, EnumConstantDecl*>` (e.g. `MissingEnumerators`)
2. Iterate over cases, deleting covered constants
3. Prepare returns true if any constant remains
4. Iterate over the map, inserting text for every element in the map


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:141
     const CaseStmt *CS = cast<CaseStmt>(CaseList);
     // Case statement covers multiple values, so just counting doesn't work.
     if (CS->caseStmtIsGNURange())
----------------
this comment no longer holds (we're not counting anymore).

Could just say // GNU range cases are rare, we don't support them


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:151
+
+    if (CE->getResultStorageKind() != ConstantExpr::RSK_Int64)
+      return false;
----------------
why can we not hit the APValue case?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:191
   for (EnumConstantDecl *Enumerator : EnumD->enumerators()) {
-    if (ExistingEnumerators.contains(Enumerator->getInitVal()))
+    // Try to insert this Enumerator into the set. If this fails, the value was
+    // either already there to begin with or we have already added it using a
----------------
Maybe just "Skip if value already covered (possibly under a different name)"?
("try to insert... if this fails" is just echoing the code)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90555