[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Please feel free to wordsmith my ABI change comment.  I don't feel strongly other than trying to make it clear that `_ExtInt` is no longer a type/types.  A question for other reviewers: Do we feel strongly enough to try to keep the old mangling around via the `clang-abi-version` flag?  I'm not motivated, as `__ExtInt` was generally experimental, but I want to make sure noone feels strongly.



================
Comment at: clang/docs/ReleaseNotes.rst:145
 
+- The mangling of the ``_ExtInt(N)`` datatype has changed in both the Microsoft
+  ABI and Itanium ABI.
----------------
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.


================
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:
> 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.


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

https://reviews.llvm.org/D108643