[PATCH] D93628: [clang] NFC: Refactor custom class into a lambda in CompilerInvocation

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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