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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
