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

[Nathan James via Phabricator via llvm-commits]

njames93 marked 6 inline comments as done.
njames93 added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:127
+  unsigned EnumIntWidth = Ctx.getIntWidth(QualType(EnumT, 0));
+  bool EnumIsSigned = EnumT->isSignedIntegerOrEnumerationType();
+
----------------
sammccall wrote:
> 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
Good point.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:129
+
+  EnumConstants.clear();
+
----------------
sammccall wrote:
> it's already clear at this point.
I didn't know how tweaks exactly worked, like if the tweak object is instantiated for each selection or if its reused. I'll remove this.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:165
+    auto Iter = EnumConstants.find(Val);
+    if (Iter == EnumConstants.end())
+      return false;
----------------
sammccall wrote:
> this says if you ever have a case statement that doesn't match an enum value, then we don't offer the tweak. Why?
If the user puts in a case statement that doesn't directly match a value, chances are they are doing something funky. In which case the tweak is likely not going to help. WDYT?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:170
+    // we encounter it.
+    if (IsCovered)
+      return false;
----------------
sammccall wrote:
> 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
What are the rules for clangd tweaks on ill-formed code.  I'll remove this anyhow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90555