[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:107
 
+- Clang now supports the _BitInt(N) family of bit-precise integer types from
+  C23. This type was previously exposed as _ExtInt(N), which is still supported
----------------
This section should probably mention the ABI break.  It shouldn't be a surprise for people upgrading.


================
Comment at: clang/docs/ReleaseNotes.rst:108
+- Clang now supports the _BitInt(N) family of bit-precise integer types from
+  C23. This type was previously exposed as _ExtInt(N), which is still supported
+  but is now deprecated in favor of the standard type. _BitInt(N) and
----------------
I would think it would be a better idea to not claim that `_ExtInt` is still supported , but perhaps just say that it is now an alias for the 'new type'.  I realize it is a little pedantic, but `_ExtInt` is no longer a type in our type system. I think this will make the diagnostics more understandable.


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1489
+  InGroup<DiagGroup<"bit-int-extension">>;
+def warn_prec2x_compat_bit_int : Warning<
+  "'_BitInt' is incompatible with C standards before C2x">,
----------------
should this be warn_cpre2x here, just to be more consistent with the 'group'?


================
Comment at: clang/include/clang/Basic/TargetInfo.h:581
   /// limitation is put into place for ABI reasons.
-  virtual bool hasExtIntType() const {
+  /// FIXME: _BitInt is a required type in C23, so there's not much utility in
+  /// asking whether the target supported it or not; I think this should be
----------------
Concur on the fixme.  I would expect after this lands that an llvm-dev discussion happen to do this alert, and have us remove this pretty quickly (a release or two?)


================
Comment at: clang/lib/AST/Type.cpp:2021
+    return IT->isSigned();
+  if (const auto *IT = dyn_cast<DependentBitIntType>(CanonicalType))
     return IT->isSigned();
----------------
Note to others: Aaron and I discussed this offline.  I don't think it is NECESSARY (as I don't think we really call these functions on dependent types), but I thought it was a good idea to be here for completeness.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:3873
                                      DiagID, Policy);
-      break;
-    case tok::kw__ExtInt: {
+      break;    
+    case tok::kw__ExtInt:
----------------
Is this extra trailing WS?


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

https://reviews.llvm.org/D108643