njames93 marked 6 inline comments as done.
njames93 added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:127
+ unsigned EnumIntWidth = Ctx.getIntWidth(QualType(EnumT, 0));
+ bool EnumIsSigned = EnumT->isSignedIntegerOrEnumerationType();
+
----------------
sammccall wrote:
> 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
Good point.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:129
+
+ EnumConstants.clear();
+
----------------
sammccall wrote:
> it's already clear at this point.
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.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:165
+ auto Iter = EnumConstants.find(Val);
+ if (Iter == EnumConstants.end())
+ return false;
----------------
sammccall wrote:
> this says if you ever have a case statement that doesn't match an enum value,
> then we don't offer the tweak. Why?
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?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp:170
+ // we encounter it.
+ if (IsCovered)
+ return false;
----------------
sammccall wrote:
> 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
What are the rules for clangd tweaks on ill-formed code. I'll remove this
anyhow.
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
