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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Fantastic, thanks!

Still LG, comments optional.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:61
 private:
+  class EnumAndCovered {
+  public:
----------------
for this kind of purpose, we'd typically just use `struct EnumAndCovered { const ECD *Enumerator; bool Covered; }` without encapsulation. But this is fine.

I think a name like `ExpectedCase` might hint a little more at the purpose. Up to you.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:165
+    auto Iter = EnumConstants.find(Val);
+    if (Iter == EnumConstants.end())
+      return false;
----------------
njames93 wrote:
> 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?
I agree it signals that exhaustively enumerating the cases *might* not be useful. (It still could be though, e.g. imagine an enum that can't easily be extended where someone's added a sentinel value).

Since this is a code action rather than a diagnostic, I'd lean towards still offering the action, but either seems reasonable.
If you're going to bail out in this case, it definitely deserves a comment.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:170
+    // we encounter it.
+    if (IsCovered)
+      return false;
----------------
njames93 wrote:
> 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.
Basically, don't crash on ill-formed code, try not to produce wildly invalid syntax.
Beyond that, we only really care about problems that come up a lot in practice.

(That said, if there are duplicated cases, I'm not sure that should stop us applying this tweak)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90555