[PATCH] D128182: [NFC] Switch FloatModeKind enum class to use bitmask enums

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

@jolanta.jensen, I was on vacation last week and missed my opportunity to comment on this. I know the change has already landed, but I added a few comments anyway.



================
Comment at: clang/include/clang/Basic/TargetInfo.h:226-227
+  unsigned RealTypeUsesObjCFPRetMask
+      : llvm::BitmaskEnumDetail::bitWidth(
+            (int)FloatModeKind::LLVM_BITMASK_LARGEST_ENUMERATOR);
   unsigned ComplexLongDoubleUsesFP2Ret : 1;
----------------
I believe this can be simplified to:
  : llvm::BitWidth<FloatModeKind>;

(It should never be necessary to reach directly into a "detail" namespace from user code)


================
Comment at: clang/include/clang/Basic/TargetInfo.h:896
   bool useObjCFPRetForRealType(FloatModeKind T) const {
-    assert(T <= FloatModeKind::Last &&
-           "T value is larger than RealTypeUsesObjCFPRetMask can handle");
-    return RealTypeUsesObjCFPRetMask & (1 << (int)T);
+    return RealTypeUsesObjCFPRetMask & llvm::BitmaskEnumDetail::Underlying(T);
   }
----------------
This again directly uses the "detail" namespace. I think this would be better written as:
  return (FloatModeKind)RealTypeUsesObjCFPRetMask & T;


================
Comment at: clang/lib/Basic/Targets/X86.h:423-424
     RealTypeUsesObjCFPRetMask =
-        ((1 << (int)FloatModeKind::Float) | (1 << (int)FloatModeKind::Double) |
-         (1 << (int)FloatModeKind::LongDouble));
+        (int)(FloatModeKind::Float | FloatModeKind::Double |
+              FloatModeKind::LongDouble);
 
----------------
Since `RealTypeUsesObjCFPRetMask` is declared as `unsigned`, it would be best to cast to `unsigned` instead of `int` here.


================
Comment at: clang/lib/Basic/Targets/X86.h:703
     // Use fpret only for long double.
-    RealTypeUsesObjCFPRetMask = (1 << (int)FloatModeKind::LongDouble);
+    RealTypeUsesObjCFPRetMask = (int)FloatModeKind::LongDouble;
 
----------------
Likewise, cast to `unsigned` here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128182/new/

https://reviews.llvm.org/D128182