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

[Sam McCall via Phabricator via llvm-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:68
+  // Maps the Enum values to the EnumConstantDecl and a bool signifying if its
+  // covered in the switch.
+  llvm::MapVector<llvm::APSInt, std::pair<const EnumConstantDecl *, bool>>
----------------
I'd make the values here a struct with named fields for readability, but up to you.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:127
+  unsigned EnumIntWidth = Ctx.getIntWidth(QualType(EnumT, 0));
+  bool EnumIsSigned = EnumT->isSignedIntegerOrEnumerationType();
+
----------------
nit: I'd add a lambda here like

```
auto Normalize(llvm::APSInt Val) {
  Val = Val.extOrTrunc(EnumIntWidth);
  Val.setIsSigned(EnumIsSigned);
  return Val;
};
```

you'll use it twice, and the callsites will be more readable


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:129
+
+  EnumConstants.clear();
+
----------------
it's already clear at this point.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:165
+    auto Iter = EnumConstants.find(Val);
+    if (Iter == EnumConstants.end())
+      return false;
----------------
this says if you ever have a case statement that doesn't match an enum value, then we don't offer the tweak. Why?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:170
+    // we encounter it.
+    if (IsCovered)
+      return false;
----------------
Do we really need to specially detect or handle this?

Seems like a blind
```
if (Iter != EnumConstants.end())
  Iter->second.second = true; // mark as covered
```
would be enough here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90555