sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:68
+ // Maps the Enum values to the EnumConstantDecl and a bool signifying if its
+ // covered in the switch.
+ llvm::MapVector<llvm::APSInt, std::pair<const EnumConstantDecl *, bool>>
----------------
I'd make the values here a struct with named fields for readability, but up to
you.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:127
+ unsigned EnumIntWidth = Ctx.getIntWidth(QualType(EnumT, 0));
+ bool EnumIsSigned = EnumT->isSignedIntegerOrEnumerationType();
+
----------------
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
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:129
+
+ EnumConstants.clear();
+
----------------
it's already clear at this point.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:165
+ auto Iter = EnumConstants.find(Val);
+ if (Iter == EnumConstants.end())
+ return false;
----------------
this says if you ever have a case statement that doesn't match an enum value,
then we don't offer the tweak. Why?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:170
+ // we encounter it.
+ if (IsCovered)
+ return false;
----------------
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
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
