[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:117
 
-  // We trigger if there are fewer cases than enum values (and no case covers
-  // multiple values). This guarantees we'll have at least one case to insert.
-  // We don't yet determine what the cases are, as that means evaluating
-  // expressions.
-  auto I = EnumD->enumerator_begin();
-  auto E = EnumD->enumerator_end();
+  // Special case of the empty enum
+  if (EnumD->enumerator_begin() == EnumD->enumerator_end())
----------------
sammccall wrote:
> why is the special case needed?
It wasn't was causing a bug early on then i fixed it and it was no longer needed


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:126
+
+  llvm::SmallMapVector<llvm::APSInt, bool, 32> EnumConstants;
+  for (auto *EnumConstant : EnumD->enumerators()) {
----------------
sammccall wrote:
> 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
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90555