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