This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Handle duplicate enum constants in PopulateSwitch tweak
ClosedPublic

Authored by njames93 on Nov 1 2020, 5:54 AM.

Details

Summary

If an enum has different names for the same constant, make sure only the first one declared gets added into the switch. Failing to do so results in a compiler error as 2 case labels can't represent the same value.

enum Numbers{
One,
Un = One,
Two,
Deux = Two,
Three,
Trois = Three
};

// Old behaviour
switch (<Number>) {
  case One:
  case Un:
  case Two:
  case Duex:
  case Three:
  case Trois: break;
}

// New behaviour
switch (<Number>) {
  case One:
  case Two:
  case Three: break;
}

Diff Detail

Event Timeline

njames93 created this revision.Nov 1 2020, 5:54 AM
njames93 requested review of this revision.Nov 1 2020, 5:54 AM
njames93 updated this revision to Diff 302191.Nov 1 2020, 4:59 PM

Fix bug where prepare would return true erroneously when enum contains duplicated constants

Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2020, 4:59 PM

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).

clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
115

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!)

117

why is the special case needed?

126

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
141

this comment no longer holds (we're not counting anymore).

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

151

why can we not hit the APValue case?

191

Maybe just "Skip if value already covered (possibly under a different name)"?
("try to insert... if this fails" is just echoing the code)

njames93 updated this revision to Diff 303502.Nov 6 2020, 11:06 AM

Addressed comments.
Now using a MapVector to collect all uncovered cases in prepare, then just loop over that in apply.

njames93 marked 6 inline comments as done.Nov 6 2020, 11:11 AM
njames93 added inline comments.
clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
117

It wasn't was causing a bug early on then i fixed it and it was no longer needed

126

Thats a great suggestion. I did alter it slightly using a std::pair<EnumConstantDecl*, bool> for the value. This is because it would be nice to catch the case where the switch has a duplicated case value. The other reason is its linear time to call erase on a MapVector, so its potentially O(N^2) when looping through all the cases.

sammccall accepted this revision.Nov 6 2020, 2:53 PM
sammccall added inline comments.
clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
68

I'd make the values here a struct with named fields for readability, but up to you.

137

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

139

it's already clear at this point.

159

this says if you ever have a case statement that doesn't match an enum value, then we don't offer the tweak. Why?

164

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

This revision is now accepted and ready to land.Nov 6 2020, 2:53 PM
njames93 marked 6 inline comments as done.Nov 6 2020, 3:58 PM
njames93 added inline comments.
clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
137

Good point.

139

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.

159

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?

164

What are the rules for clangd tweaks on ill-formed code. I'll remove this anyhow.

njames93 updated this revision to Diff 303580.Nov 6 2020, 3:59 PM
njames93 marked 3 inline comments as done.

Address comments

sammccall accepted this revision.Nov 9 2020, 3:01 AM

Fantastic, thanks!

Still LG, comments optional.

clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp
62

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.

159

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.

164

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)

This revision was landed with ongoing or failed builds.Nov 9 2020, 4:15 AM
This revision was automatically updated to reflect the committed changes.