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