sammccall added a comment.
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).
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:115
- // 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.
----------------
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!)
================
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())
----------------
why is the special case needed?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:126
+
+ llvm::SmallMapVector<llvm::APSInt, bool, 32> EnumConstants;
+ for (auto *EnumConstant : EnumD->enumerators()) {
----------------
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
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:141
const CaseStmt *CS = cast<CaseStmt>(CaseList);
// Case statement covers multiple values, so just counting doesn't work.
if (CS->caseStmtIsGNURange())
----------------
this comment no longer holds (we're not counting anymore).
Could just say // GNU range cases are rare, we don't support them
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:151
+
+ if (CE->getResultStorageKind() != ConstantExpr::RSK_Int64)
+ return false;
----------------
why can we not hit the APValue case?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:191
for (EnumConstantDecl *Enumerator : EnumD->enumerators()) {
- if (ExistingEnumerators.contains(Enumerator->getInitVal()))
+ // Try to insert this Enumerator into the set. If this fails, the value was
+ // either already there to begin with or we have already added it using a
----------------
Maybe just "Skip if value already covered (possibly under a different name)"?
("try to insert... if this fails" is just echoing the code)
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
