[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:145
 
+- The mangling of the ``_ExtInt(N)`` datatype has changed in both the Microsoft
+  ABI and Itanium ABI.
----------------
erichkeane wrote:
> Hrm... not a huge fan that this still claims that ``_ExtInt`` is a type(though don't have a better wording), but I'd also probably want to mention that it now matches the new type.  Perhaps something like:
> 
>     The ``_ExtInt(N)`` family of types have been replaced with the C2X standardized version of the feature, ``_BitInt(N)``.  Therefore, source that previously used the ``_ExtInt`` types will now be mangled to instead use the ``_BitInt(N)`` spelling in both the Microsoft and Itanium ABIs.
I can go with this.


================
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
----------------
erichkeane wrote:
> erichkeane wrote:
> > 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?)
> To clarify: This should be removed at the beginning of a release-cycle (along with an llvm-dev notice) so that we have as much time as possible in trunk to react/find issues.
We're basically at the beginning of Clang 14 (13 isn't out the door yet), so I am sort of tempted to alert llvm-dev now and remove it as part of this review. However, if people think that's too harsh, I'm happy to wait as well.


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

https://reviews.llvm.org/D108643