[PATCH] D93628: [clang] NFC: Refactor custom class into a lambda in CompilerInvocation
Duncan P. N. Exon Smith via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 21 13:07:13 PST 2020
dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.
This looks like an improvement, so LGTM, but I have a couple of comments.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:160-161
-namespace {
-template <typename T> struct FlagToValueNormalizer {
- T Value;
-
- Optional<T> operator()(OptSpecifier Opt, unsigned, const ArgList &Args,
- DiagnosticsEngine &) {
+template <typename T, std::enable_if_t<!(!std::is_same<T, uint64_t>::value &&
+ llvm::is_integral_or_enum<T>::value),
+ bool> = false>
----------------
I missed the double negation here for a while; might be simpler as:
```
<std::is_same<T, uint64_t>::value ||
!llvm::is_integral_or_neum<T>::value>
```
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:172-174
+template <typename T, std::enable_if_t<(!std::is_same<T, uint64_t>::value &&
+ llvm::is_integral_or_enum<T>::value),
+ bool> = false>
----------------
Ah, I guess you want this to match the above. I guess that's the benefit of the old `is_int_convertible` -- it didn't need double-negation to make it clear that the two functions were alternatives. I might prefer the previous option of having a stand-alone function (possibly renamed if you think the old name isn't correct anymore). (Regardless, you can drop the unnecessary parentheses.)
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:176
+static auto makeFlagToValueNormalizer(T Value) {
+ return makeFlagToValueNormalizer(uint64_t(Value));
}
----------------
(not a behaviour change but) I wonder if this correct for signed types, where the conversion back to `T` could change sign if it's a negative value. Should there be an assertion that the value is `>= 0`? (Probably to do separately / outside this patch)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93628/new/
https://reviews.llvm.org/D93628
More information about the cfe-commits
mailing list