[clang][Sema] Move computing enum bits into a separate function

[kuilpd]

Move the code that computes NumNegativeBits and NumPositiveBits for an enum to a separate function in ASTContext.h.
This function needs to be called from LLDB as well (#115005)

[kuilpd]

The code needs to be called from LLDB as well (#115005), so this is an NFC patch to move it to ASTContext.h, plus this cleans up a bit the code in Sema::ActOnEnumBody.
isRepresentableIntegerValue also had to be moved to ASTContext.h, but since it requires ASTContext as a first argument anyway, it seems like it's a better location for this function anyway.
The only question is that computeEnumBits has a raw template in order to function with different collections that Sema and LLDB use, so the function has to be defined in the header. Can this be a problem?

[llvmbot]

@llvm/pr-subscribers-clang

Author: Ilia Kuklin (kuilpd)

Changes

Move the code that computes NumNegativeBits and NumPositiveBits for an enum to a separate function in ASTContext.h

---

Full diff: https://github.com/llvm/llvm-project/pull/126096.diff

3 Files Affected:

• (modified) clang/include/clang/AST/ASTContext.h (+41)
• (modified) clang/lib/AST/ASTContext.cpp (+13)
• (modified) clang/lib/Sema/SemaDecl.cpp (+5-49)

diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 65be782c1ba43e8..a96b9c0a17045af 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -1733,6 +1733,47 @@ class ASTContext : public RefCountedBase<ASTContext> {
                             unsigned NumPositiveBits, QualType &BestType,
                             QualType &BestPromotionType);
 
+  /// Determine whether the given integral value is representable within
+  /// the given type T.
+  bool isRepresentableIntegerValue(llvm::APSInt &Value, QualType T);
+
+  /// Compute NumNegativeBits and NumPositiveBits for an enum based on
+  /// the constant values of its enumerators.
+  template <typename RangeT>
+  bool computeEnumBits(RangeT EnumConstants, unsigned &NumNegativeBits,
+                       unsigned &NumPositiveBits) {
+    NumNegativeBits = 0;
+    NumPositiveBits = 0;
+    bool MembersRepresentableByInt = true;
+    for (auto *Elem : EnumConstants) {
+      EnumConstantDecl *ECD = cast_or_null<EnumConstantDecl>(Elem);
+      if (!ECD)
+        continue; // Already issued a diagnostic.
+
+      llvm::APSInt InitVal = ECD->getInitVal();
+      if (InitVal.isUnsigned() || InitVal.isNonNegative()) {
+        // If the enumerator is zero that should still be counted as a positive
+        // bit since we need a bit to store the value zero.
+        unsigned ActiveBits = InitVal.getActiveBits();
+        NumPositiveBits = std::max({NumPositiveBits, ActiveBits, 1u});
+      } else {
+        NumNegativeBits =
+            std::max(NumNegativeBits, (unsigned)InitVal.getSignificantBits());
+      }
+
+      MembersRepresentableByInt &= isRepresentableIntegerValue(InitVal, IntTy);
+    }
+
+    // If we have an empty set of enumerators we still need one bit.
+    // From [dcl.enum]p8
+    // If the enumerator-list is empty, the values of the enumeration are as if
+    // the enumeration had a single enumerator with value 0
+    if (!NumPositiveBits && !NumNegativeBits)
+      NumPositiveBits = 1;
+
+    return MembersRepresentableByInt;
+  }
+
   QualType
   getUnresolvedUsingType(const UnresolvedUsingTypenameDecl *Decl) const;
 
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index de617860b70040b..3a2ccd3d2ddcbf4 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -5320,6 +5320,19 @@ bool ASTContext::computeBestEnumTypes(bool IsPacked, unsigned NumNegativeBits,
   return EnumTooLarge;
 }
 
+bool ASTContext::isRepresentableIntegerValue(llvm::APSInt &Value, QualType T) {
+  assert((T->isIntegralType(*this) || T->isEnumeralType()) &&
+         "Integral type required!");
+  unsigned BitWidth = getIntWidth(T);
+
+  if (Value.isUnsigned() || Value.isNonNegative()) {
+    if (T->isSignedIntegerOrEnumerationType())
+      --BitWidth;
+    return Value.getActiveBits() <= BitWidth;
+  }
+  return Value.getSignificantBits() <= BitWidth;
+}
+
 QualType ASTContext::getUnresolvedUsingType(
     const UnresolvedUsingTypenameDecl *Decl) const {
   if (Decl->TypeForDecl)
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 74e0fcec2d911bc..6eedc77ed20a09e 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -19633,23 +19633,6 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl,
   ProcessAPINotes(Record);
 }
 
-/// Determine whether the given integral value is representable within
-/// the given type T.
-static bool isRepresentableIntegerValue(ASTContext &Context,
-                                        llvm::APSInt &Value,
-                                        QualType T) {
-  assert((T->isIntegralType(Context) || T->isEnumeralType()) &&
-         "Integral type required!");
-  unsigned BitWidth = Context.getIntWidth(T);
-
-  if (Value.isUnsigned() || Value.isNonNegative()) {
-    if (T->isSignedIntegerOrEnumerationType())
-      --BitWidth;
-    return Value.getActiveBits() <= BitWidth;
-  }
-  return Value.getSignificantBits() <= BitWidth;
-}
-
 // Given an integral type, return the next larger integral type
 // (or a NULL type of no such type exists).
 static QualType getNextLargerIntegralType(ASTContext &Context, QualType T) {
@@ -19723,7 +19706,7 @@ EnumConstantDecl *Sema::CheckEnumConstant(EnumDecl *Enum,
           // representable in the underlying type of the enumeration. In C++11,
           // we perform a non-narrowing conversion as part of converted constant
           // expression checking.
-          if (!isRepresentableIntegerValue(Context, EnumVal, EltTy)) {
+          if (!Context.isRepresentableIntegerValue(EnumVal, EltTy)) {
             if (Context.getTargetInfo()
                     .getTriple()
                     .isWindowsMSVCEnvironment()) {
@@ -19752,7 +19735,7 @@ EnumConstantDecl *Sema::CheckEnumConstant(EnumDecl *Enum,
           //   representable as an int.
 
           // Complain if the value is not representable in an int.
-          if (!isRepresentableIntegerValue(Context, EnumVal, Context.IntTy)) {
+          if (!Context.isRepresentableIntegerValue(EnumVal, Context.IntTy)) {
             Diag(IdLoc, getLangOpts().C23
                             ? diag::warn_c17_compat_enum_value_not_int
                             : diag::ext_c23_enum_value_not_int)
@@ -19844,7 +19827,7 @@ EnumConstantDecl *Sema::CheckEnumConstant(EnumDecl *Enum,
                           : diag::ext_c23_enum_value_not_int)
               << 1 << toString(EnumVal, 10) << 1;
       } else if (!getLangOpts().CPlusPlus && !EltTy->isDependentType() &&
-                 !isRepresentableIntegerValue(Context, EnumVal, EltTy)) {
+                 !Context.isRepresentableIntegerValue(EnumVal, EltTy)) {
         // Enforce C99 6.7.2.2p2 even when we compute the next value.
         Diag(IdLoc, getLangOpts().C23 ? diag::warn_c17_compat_enum_value_not_int
                                       : diag::ext_c23_enum_value_not_int)
@@ -20171,35 +20154,8 @@ void Sema::ActOnEnumBody(SourceLocation EnumLoc, SourceRange BraceRange,
   // reverse the list.
   unsigned NumNegativeBits = 0;
   unsigned NumPositiveBits = 0;
-  bool MembersRepresentableByInt = true;
-
-  for (unsigned i = 0, e = Elements.size(); i != e; ++i) {
-    EnumConstantDecl *ECD =
-      cast_or_null<EnumConstantDecl>(Elements[i]);
-    if (!ECD) continue;  // Already issued a diagnostic.
-
-    llvm::APSInt InitVal = ECD->getInitVal();
-
-    // Keep track of the size of positive and negative values.
-    if (InitVal.isUnsigned() || InitVal.isNonNegative()) {
-      // If the enumerator is zero that should still be counted as a positive
-      // bit since we need a bit to store the value zero.
-      unsigned ActiveBits = InitVal.getActiveBits();
-      NumPositiveBits = std::max({NumPositiveBits, ActiveBits, 1u});
-    } else {
-      NumNegativeBits =
-          std::max(NumNegativeBits, (unsigned)InitVal.getSignificantBits());
-    }
-    MembersRepresentableByInt &=
-        isRepresentableIntegerValue(Context, InitVal, Context.IntTy);
-  }
-
-  // If we have an empty set of enumerators we still need one bit.
-  // From [dcl.enum]p8
-  // If the enumerator-list is empty, the values of the enumeration are as if
-  // the enumeration had a single enumerator with value 0
-  if (!NumPositiveBits && !NumNegativeBits)
-    NumPositiveBits = 1;
+  bool MembersRepresentableByInt =
+      Context.computeEnumBits(Elements, NumNegativeBits, NumPositiveBits);
 
   // Figure out the type that should be used for this enum.
   QualType BestType;

[Sirraide]

This is just moving stuff around so that seems fine. Though, I wonder, could we just pass e.g. an ArrayRef<Decl*> to computeEnumBits()? The LLDB pr you linked seems to have an SmallVector<EnumConstantDecl*>, so that should be possible (I think it does require a reinterpret_cast from a const Decl** to a const EnumConstantDecl**, but we already do that in other places in Clang iirc).

[AaronBallman]

The code needs to be called from LLDB as well (#115005),

Please add this context to the patch summary so that it explains why the changes are needed, not just what the changes are doing.

[Michael137]

This is just moving stuff around so that seems fine. Though, I wonder, could we just pass e.g. an ArrayRef<Decl*> to computeEnumBits()? The LLDB pr you linked seems to have an SmallVector<EnumConstantDecl*>, so that should be possible (I think it does require a reinterpret_cast from a const Decl** to a const EnumConstantDecl**, but we already do that in other places in Clang iirc).

The reason I suggested the template initially was because in LLDB's case we don't have the EnumConstantDecls in a container. We only have an EnumDecl. So the plan was to pass the enumerators() iterator into computeEnumBits. But if you know of a good way to turn the range into a container that we can pass as an ArrayRef, or some non-template way of passing both an iterator_range and ArrayRef into computeEnumBits, that'd be great!

[kuilpd]

This is just moving stuff around so that seems fine. Though, I wonder, could we just pass e.g. an ArrayRef<Decl*> to computeEnumBits()? The LLDB pr you linked seems to have an SmallVector<EnumConstantDecl*>, so that should be possible (I think it does require a reinterpret_cast from a const Decl** to a const EnumConstantDecl**, but we already do that in other places in Clang iirc).

The reason I suggested the template initially was because in LLDB's case we don't have the EnumConstantDecls in a container. We only have an EnumDecl. So the plan was to pass the enumerators() iterator into computeEnumBits. But if you know of a good way to turn the range into a container that we can pass as an ArrayRef, or some non-template way of passing both an iterator_range and ArrayRef into computeEnumBits, that'd be great!

Yeah, enumerator() returns llvm::iterator_range<specific_decl_iterator<EnumConstantDecl>>, and couldn't find a good way to convert this to an ArrayRef.
I guess I could make a SmallVector<EnumConstantDecl*> and put the copy of pointers there at the point of their creation (here), since the function return a EnumConstantDecl, then discard it.

[Sirraide]

This is just moving stuff around so that seems fine. Though, I wonder, could we just pass e.g. an ArrayRef<Decl*> to computeEnumBits()? The LLDB pr you linked seems to have an SmallVector<EnumConstantDecl*>, so that should be possible (I think it does require a reinterpret_cast from a const Decl** to a const EnumConstantDecl**, but we already do that in other places in Clang iirc).

The reason I suggested the template initially was because in LLDB's case we don't have the EnumConstantDecls in a container. We only have an EnumDecl. So the plan was to pass the enumerators() iterator into computeEnumBits. But if you know of a good way to turn the range into a container that we can pass as an ArrayRef, or some non-template way of passing both an iterator_range and ArrayRef into computeEnumBits, that'd be great!

Yeah, enumerator() returns llvm::iterator_range<specific_decl_iterator<EnumConstantDecl>>, and couldn't find a good way to convert this to an ArrayRef. I guess I could make a SmallVector<EnumConstantDecl*> and put the copy of pointers there at the point of their creation (here), since the function return a EnumConstantDecl, then discard it.

Hmm, I feel like you might be able to extract the pointers from the range and do something with those, but I’m not too familiar w/ how that works exactly off the top of my head. I don’t think the template is that big of an issue; it’d be nice not to have it, but if there is no sane/straight-forward way to get around it then I don’t really have a problem w/ it either.

[Michael137]

This is just moving stuff around so that seems fine. Though, I wonder, could we just pass e.g. an ArrayRef<Decl*> to computeEnumBits()? The LLDB pr you linked seems to have an SmallVector<EnumConstantDecl*>, so that should be possible (I think it does require a reinterpret_cast from a const Decl** to a const EnumConstantDecl**, but we already do that in other places in Clang iirc).

The reason I suggested the template initially was because in LLDB's case we don't have the EnumConstantDecls in a container. We only have an EnumDecl. So the plan was to pass the enumerators() iterator into computeEnumBits. But if you know of a good way to turn the range into a container that we can pass as an ArrayRef, or some non-template way of passing both an iterator_range and ArrayRef into computeEnumBits, that'd be great!

Yeah, enumerator() returns llvm::iterator_range<specific_decl_iterator<EnumConstantDecl>>, and couldn't find a good way to convert this to an ArrayRef. I guess I could make a SmallVector<EnumConstantDecl*> and put the copy of pointers there at the point of their creation (here), since the function return a EnumConstantDecl, then discard it.

Hmm, I feel like you might be able to extract the pointers from the range and do something with those, but I’m not too familiar w/ how that works exactly off the top of my head. I don’t think the template is that big of an issue; it’d be nice not to have it, but if there is no sane/straight-forward way to get around it then I don’t really have a problem w/ it either.

The only way I can see us extracting a raw clang::Decl* out of the iterator is if we somehow get to DeclContext::FirstDecl/DeclContext::LastDecl, and construct the ArrayRef from those. But I don't see a good way of doing that. The alternative is to collect all the enumerators into a temporary SmallVector on the LLDB side, then pass it as an ArrayRef, though that feels a bit silly since we're in control of the interface and could just avoid the work. So my personal preference is to stick with the template, but I'll leave it to the Clang maintainers for the final call

[Sirraide]

Makes sense. Maybe wait a bit and see if anyone else has reservations about this, but from my point of view it’s fine as-is.

[Fznamznon]

I don't have any problem with this either.