[Clang] Remove redundant check for scoped enums in shift operators #152865

tinnamchoi · 2025-08-09T16:36:57Z

This check, introduced in 0bf3140, has not been necessary since 21673c4

llvm-project/clang/lib/Sema/SemaExpr.cpp

Lines 5432 to 5442 in 0bf3140

    
           // C99 6.5.7p2: Each of the operands shall have integer type. 
        
           if (!lex->getType()->hasIntegerRepresentation() ||  
        
               !rex->getType()->hasIntegerRepresentation()) 
        
             return InvalidOperands(Loc, lex, rex); 
        
           // C++0x: Don't allow scoped enums. FIXME: Use something better than 
        
           // hasIntegerRepresentation() above instead of this. 
        
           if (isScopedEnumerationType(lex->getType()) || 
        
               isScopedEnumerationType(rex->getType())) { 
        
             return InvalidOperands(Loc, lex, rex); 
        
           }

llvm-project/clang/lib/AST/Type.cpp

Lines 513 to 529 in 21673c4

    
           bool Type::isIntegerType() const { 
        
             if (const BuiltinType *BT = dyn_cast<BuiltinType>(CanonicalType)) 
        
               return BT->getKind() >= BuiltinType::Bool && 
        
                      BT->getKind() <= BuiltinType::Int128; 
        
             if (const EnumType *ET = dyn_cast<EnumType>(CanonicalType)) 
        
               // Incomplete enum types are not treated as integer types. 
        
               // FIXME: In C++, enum types are never integer types. 
        
               return ET->getDecl()->isComplete() && !ET->getDecl()->isScoped(); 
        
             return false; 
        
           } 
        
           bool Type::hasIntegerRepresentation() const { 
        
             if (const VectorType *VT = dyn_cast<VectorType>(CanonicalType)) 
        
               return VT->getElementType()->isIntegerType(); 
        
             else 
        
               return isIntegerType(); 
        
           }

This check, introduced in 0bf3140, has not been necessary since 21673c4

github-actions · 2025-08-09T16:37:16Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-08-09T16:37:49Z

@llvm/pr-subscribers-clang

Author: Timothy Choi (tinnamchoi)

Changes

This check, introduced in 0bf3140, has not been necessary since 21673c4

llvm-project/clang/lib/Sema/SemaExpr.cpp

Lines 5432 to 5442 in 0bf3140

    
           // C99 6.5.7p2: Each of the operands shall have integer type. 
        
           if (!lex->getType()->hasIntegerRepresentation() ||  
        
               !rex->getType()->hasIntegerRepresentation()) 
        
             return InvalidOperands(Loc, lex, rex); 
        
           // C++0x: Don't allow scoped enums. FIXME: Use something better than 
        
           // hasIntegerRepresentation() above instead of this. 
        
           if (isScopedEnumerationType(lex->getType()) || 
        
               isScopedEnumerationType(rex->getType())) { 
        
             return InvalidOperands(Loc, lex, rex); 
        
           }

llvm-project/clang/lib/AST/Type.cpp

Lines 513 to 529 in 21673c4

    
           bool Type::isIntegerType() const { 
        
             if (const BuiltinType *BT = dyn_cast<BuiltinType>(CanonicalType)) 
        
               return BT->getKind() >= BuiltinType::Bool && 
        
                      BT->getKind() <= BuiltinType::Int128; 
        
             if (const EnumType *ET = dyn_cast<EnumType>(CanonicalType)) 
        
               // Incomplete enum types are not treated as integer types. 
        
               // FIXME: In C++, enum types are never integer types. 
        
               return ET->getDecl()->isComplete() && !ET->getDecl()->isScoped(); 
        
             return false; 
        
           } 
        
           bool Type::hasIntegerRepresentation() const { 
        
             if (const VectorType *VT = dyn_cast<VectorType>(CanonicalType)) 
        
               return VT->getElementType()->isIntegerType(); 
        
             else 
        
               return isIntegerType(); 
        
           }

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

1 Files Affected:

(modified) clang/lib/Sema/SemaExpr.cpp (-6)

diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 6793d6da85cb1..c805ef205beba 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -11747,12 +11747,6 @@ QualType Sema::CheckShiftOperands(ExprResult &LHS, ExprResult &RHS,
       !RHSType->hasIntegerRepresentation())
     return InvalidOperands(Loc, LHS, RHS);
 
-  // C++0x: Don't allow scoped enums. FIXME: Use something better than
-  // hasIntegerRepresentation() above instead of this.
-  if (isScopedEnumerationType(LHSType) ||
-      isScopedEnumerationType(RHSType)) {
-    return InvalidOperands(Loc, LHS, RHS);
-  }
   DiagnoseBadShiftValues(*this, LHS, RHS, Loc, Opc, LHSType);
 
   // "The type of the result is that of the promoted left operand."

Sirraide

Yeah, I think this is fine because we don’t treat scoped enums as integer types, but we don’t seem to have any tests for this, so could you add some to make sure that we do reject shifts where one (or both) operands are scoped enums (in C and C++).

tinnamchoi · 2025-08-10T00:08:26Z

What would the tests in C look like? Scoped enums don't exist in C.

Other than that, the tests to be introduced in #152698 should cover this, running locally seems fine.

llvm-project/clang/test/SemaCXX/enum-scoped.cpp

Lines 439 to 452 in 45c8c06

    
           0 << E_int::e; // expected-error {{invalid operands to binary expression ('int' and 'GH24265::E_int')}} 
        
                          // cxx11-20-note@-1 {{no implicit conversion for scoped enum; consider casting to underlying type}} 
        
                          // CXX20: fix-it:{{.*}}:{[[@LINE-2]]:10-[[@LINE-2]]:10}:"static_cast<int>(" 
        
                          // CXX20: fix-it:{{.*}}:{[[@LINE-3]]:17-[[@LINE-3]]:17}:")" 
        
                          // cxx23-note@-4 {{no implicit conversion for scoped enum; consider using std::to_underlying}} 
        
                          // CXX23: fix-it:{{.*}}:{[[@LINE-5]]:10-[[@LINE-5]]:10}:"std::to_underlying(" 
        
                          // CXX23: fix-it:{{.*}}:{[[@LINE-6]]:17-[[@LINE-6]]:17}:")" 
        
           0 >> E_int::e; // expected-error {{invalid operands to binary expression ('int' and 'GH24265::E_int')}} 
        
                          // cxx11-20-note@-1 {{no implicit conversion for scoped enum; consider casting to underlying type}} 
        
                          // CXX20: fix-it:{{.*}}:{[[@LINE-2]]:10-[[@LINE-2]]:10}:"static_cast<int>(" 
        
                          // CXX20: fix-it:{{.*}}:{[[@LINE-3]]:17-[[@LINE-3]]:17}:")" 
        
                          // cxx23-note@-4 {{no implicit conversion for scoped enum; consider using std::to_underlying}} 
        
                          // CXX23: fix-it:{{.*}}:{[[@LINE-5]]:10-[[@LINE-5]]:10}:"std::to_underlying(" 
        
                          // CXX23: fix-it:{{.*}}:{[[@LINE-6]]:17-[[@LINE-6]]:17}:")"

llvm-project/clang/test/SemaCXX/enum-scoped.cpp

Lines 580 to 593 in 45c8c06

    
           a <<= E_int::e; // expected-error {{invalid operands to binary expression ('int' and 'GH24265::E_int')}} 
        
                           // cxx11-20-note@-1 {{no implicit conversion for scoped enum; consider casting to underlying type}} 
        
                           // CXX20: fix-it:{{.*}}:{[[@LINE-2]]:11-[[@LINE-2]]:11}:"static_cast<int>(" 
        
                           // CXX20: fix-it:{{.*}}:{[[@LINE-3]]:18-[[@LINE-3]]:18}:")" 
        
                           // cxx23-note@-4 {{no implicit conversion for scoped enum; consider using std::to_underlying}} 
        
                           // CXX23: fix-it:{{.*}}:{[[@LINE-5]]:11-[[@LINE-5]]:11}:"std::to_underlying(" 
        
                           // CXX23: fix-it:{{.*}}:{[[@LINE-6]]:18-[[@LINE-6]]:18}:")" 
        
           a >>= E_int::e; // expected-error {{invalid operands to binary expression ('int' and 'GH24265::E_int')}} 
        
                           // cxx11-20-note@-1 {{no implicit conversion for scoped enum; consider casting to underlying type}} 
        
                           // CXX20: fix-it:{{.*}}:{[[@LINE-2]]:11-[[@LINE-2]]:11}:"static_cast<int>(" 
        
                           // CXX20: fix-it:{{.*}}:{[[@LINE-3]]:18-[[@LINE-3]]:18}:")" 
        
                           // cxx23-note@-4 {{no implicit conversion for scoped enum; consider using std::to_underlying}} 
        
                           // CXX23: fix-it:{{.*}}:{[[@LINE-5]]:11-[[@LINE-5]]:11}:"std::to_underlying(" 
        
                           // CXX23: fix-it:{{.*}}:{[[@LINE-6]]:18-[[@LINE-6]]:18}:")"

llvm-project/clang/test/SemaCXX/enum-scoped.cpp

Lines 623 to 624 in 45c8c06

    
           b <<= 0; // expected-error {{invalid operands to binary expression ('E_int' and 'int')}} 
        
           b >>= 0; // expected-error {{invalid operands to binary expression ('E_int' and 'int')}}

Sirraide · 2025-08-10T07:03:49Z

What would the tests in C look like? Scoped enums don't exist in C.

Yeah, ignore that; I was already tired yesterday and for some reason thought C had them too (it doesn’t; what it does have now is enums with a fixed underlying type, which is probably what I was thinking of)

Other than that, the tests to be introduced in #152698 should cover this, running locally seems fine.

I mean, it might honestly make more sense then to just integrate this change into that pr seeing as this one is pretty small and you’re basically already refactoring how checking around scoped enums works in the other one.

tinnamchoi · 2025-08-10T08:01:58Z

I prefer keeping my PRs atomic. All of my changes in that PR are necessary towards the primary goal of that PR, whereas this change wouldn't be. If I allowed myself to make refactors whenever I'm working on a feature it would introduce quite a bit of noise, which seems like a bad habit to have.

Sirraide · 2025-08-10T08:05:17Z

I prefer keeping my PRs atomic. All of my changes in that PR are necessary towards the primary goal of that PR, whereas this change wouldn't be. If I allowed myself to make refactors whenever I'm working on a feature it would introduce quite a bit of noise, which seems like a bad habit to have.

I mean, altenratively we could just wait until the other pr is merged and then merge this one afterwards; I just don’t like the idea of making a non-obvious change if there are no tests at all for it (unless we already have test that I missed but I couldn’t find any).

tinnamchoi · 2025-08-10T08:18:05Z

Agreed, let's wait for that then.

[Clang] Remove redundant check for scoped enums in shift operators

fc0a4c7

This check, introduced in 0bf3140, has not been necessary since 21673c4

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Aug 9, 2025

Sirraide reviewed Aug 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang] Remove redundant check for scoped enums in shift operators #152865

[Clang] Remove redundant check for scoped enums in shift operators #152865

tinnamchoi commented Aug 9, 2025

Uh oh!

github-actions bot commented Aug 9, 2025

Uh oh!

llvmbot commented Aug 9, 2025

Uh oh!

Sirraide left a comment

Uh oh!

tinnamchoi commented Aug 10, 2025

Uh oh!

Sirraide commented Aug 10, 2025

Uh oh!

tinnamchoi commented Aug 10, 2025

Uh oh!

Sirraide commented Aug 10, 2025

Uh oh!

tinnamchoi commented Aug 10, 2025

Uh oh!

Uh oh!

	// C99 6.5.7p2: Each of the operands shall have integer type.
	if (!lex->getType()->hasIntegerRepresentation() \|\|
	!rex->getType()->hasIntegerRepresentation())
	return InvalidOperands(Loc, lex, rex);

	// C++0x: Don't allow scoped enums. FIXME: Use something better than
	// hasIntegerRepresentation() above instead of this.
	if (isScopedEnumerationType(lex->getType()) \|\|
	isScopedEnumerationType(rex->getType())) {
	return InvalidOperands(Loc, lex, rex);
	}

	bool Type::isIntegerType() const {
	if (const BuiltinType *BT = dyn_cast<BuiltinType>(CanonicalType))
	return BT->getKind() >= BuiltinType::Bool &&
	BT->getKind() <= BuiltinType::Int128;
	if (const EnumType *ET = dyn_cast<EnumType>(CanonicalType))
	// Incomplete enum types are not treated as integer types.
	// FIXME: In C++, enum types are never integer types.
	return ET->getDecl()->isComplete() && !ET->getDecl()->isScoped();
	return false;
	}

	bool Type::hasIntegerRepresentation() const {
	if (const VectorType *VT = dyn_cast<VectorType>(CanonicalType))
	return VT->getElementType()->isIntegerType();
	else
	return isIntegerType();
	}

[Clang] Remove redundant check for scoped enums in shift operators #152865

Are you sure you want to change the base?

[Clang] Remove redundant check for scoped enums in shift operators #152865

Conversation

tinnamchoi commented Aug 9, 2025

Uh oh!

github-actions bot commented Aug 9, 2025

Uh oh!

llvmbot commented Aug 9, 2025

Uh oh!

Sirraide left a comment

Choose a reason for hiding this comment

Uh oh!

tinnamchoi commented Aug 10, 2025

Uh oh!

Sirraide commented Aug 10, 2025

Uh oh!

tinnamchoi commented Aug 10, 2025

Uh oh!

Sirraide commented Aug 10, 2025

Uh oh!

tinnamchoi commented Aug 10, 2025

Uh oh!

Uh oh!