[cfe-dev] RFC: Defaulted comparison operators

Oleg Smolsky oleg.smolsky at riverbed.com
Tue Mar 11 07:41:55 PDT 2014


Hi all, this proposal is live on the ISO site:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3950.html

Could I get comments on the first half of the patch please? It is 
functional, but I barely understand Clang's internals and there are a 
few ToDo statements there...

Thanks!
Oleg.

On 2013-12-26 19:06, Oleg Smolsky wrote:
> Hi all, I've started working on an ISO C++ proposal (attached) and 
> would like to ask for feedback on the prototype implementation 
> (attached).
>
> The point here is to extend the notion of "special member functions" 
> to include operator==() and its boolean inverse. The patch is minimal 
> and has a few "FIXME:" tags due to the lack of Clang knowledge...
>
> Could you folks review and comment please?
>
> Thanks in advance,
> Oleg.

-------------- next part --------------
commit ff30be8902fdbb2397a471bc8dea13dbe4d84b59
Author: Oleg Smolsky <oleg at smolsky.net>
Date:   Mon Dec 23 20:08:11 2013 -0800

    First cut of (in)equality operators
    
     - works for built-in and composite (user-defined) types
     - minimal validity checks
    
    ToDo:
     - need to fix a few FIXME tags

diff --git a/include/clang/AST/DeclCXX.h b/include/clang/AST/DeclCXX.h
index 75f20e2..f42b84c 100644
--- a/include/clang/AST/DeclCXX.h
+++ b/include/clang/AST/DeclCXX.h
@@ -1712,6 +1712,16 @@ public:
   /// \brief Determine whether this is a move assignment operator.
   bool isMoveAssignmentOperator() const;
 
+  /// \brief Determine whether this is an equality operator.
+  bool isEqualityOperator() const {
+    return getOverloadedOperator() == OO_EqualEqual && !isStatic();
+  }
+
+  /// \brief Determine whether this is an inequality operator.
+  bool isInequalityOperator() const {
+    return getOverloadedOperator() == OO_ExclaimEqual && !isStatic();
+  }
+
   CXXMethodDecl *getCanonicalDecl() {
     return cast<CXXMethodDecl>(FunctionDecl::getCanonicalDecl());
   }
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 838ce63..09de6d2 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6111,6 +6111,8 @@ def err_out_of_line_default_deletes : Error<
   "defaulting this %select{default constructor|copy constructor|move "
   "constructor|copy assignment operator|move assignment operator|destructor}0 "
   "would delete it after its first declaration">;
+def err_defaulted_comp_member_return_type : Error<
+  "explicitly-defaulted %0 operator must return %1">;
 def warn_vbase_moved_multiple_times : Warning<
   "defaulted move assignment operator of %0 will move assign virtual base "
   "class %1 multiple times">, InGroup<DiagGroup<"multiple-move-vbase">>;
diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index 8533972..d0d8328 100644
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -819,6 +819,8 @@ public:
     CXXCopyAssignment,
     CXXMoveAssignment,
     CXXDestructor,
+    CXXEqualityOperator,
+    CXXInequalityOperator,
     CXXInvalid
   };
 
@@ -3953,6 +3955,14 @@ public:
   void DefineImplicitCopyAssignment(SourceLocation CurrentLocation,
                                     CXXMethodDecl *MethodDecl);
 
+  /// \brief Defines a default-declared equality operator.
+  void DefineDefaultEquality(SourceLocation CurrentLocation,
+                             CXXMethodDecl *MethodDecl);
+
+  /// \brief Defines a default-declared inquality operator.
+  void DefineDefaultInequality(SourceLocation CurrentLocation,
+                               CXXMethodDecl *MethodDecl);
+
   /// \brief Declare the implicit move assignment operator for the given class.
   ///
   /// \param ClassDecl The Class declaration into which the implicit
diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
index 30ec0c4..ab1c995 100644
--- a/lib/Sema/SemaDecl.cpp
+++ b/lib/Sema/SemaDecl.cpp
@@ -2248,6 +2248,10 @@ Sema::CXXSpecialMember Sema::getSpecialMember(const CXXMethodDecl *MD) {
     return Sema::CXXCopyAssignment;
   } else if (MD->isMoveAssignmentOperator()) {
     return Sema::CXXMoveAssignment;
+  } else if (MD->isEqualityOperator()) {
+    return Sema::CXXEqualityOperator;
+  } else if (MD->isInequalityOperator()) {
+    return Sema::CXXInequalityOperator;
   }
 
   return Sema::CXXInvalid;
diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp
index 117c37b..35dd28b 100644
--- a/lib/Sema/SemaDeclCXX.cpp
+++ b/lib/Sema/SemaDeclCXX.cpp
@@ -37,6 +37,7 @@
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
+#include "clang/Sema/SemaDiagnostic.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include <map>
@@ -4614,6 +4615,8 @@ static bool defaultedSpecialMemberIsConstexpr(Sema &S, CXXRecordDecl *ClassDecl,
     Ctor = false;
     break;
 
+  case Sema::CXXEqualityOperator:
+  case Sema::CXXInequalityOperator:
   case Sema::CXXDestructor:
   case Sema::CXXInvalid:
     return false;
@@ -4694,6 +4697,11 @@ computeImplicitExceptionSpec(Sema &S, SourceLocation Loc, CXXMethodDecl *MD) {
     return S.ComputeDefaultedMoveAssignmentExceptionSpec(MD);
   case Sema::CXXDestructor:
     return S.ComputeDefaultedDtorExceptionSpec(MD);
+  case Sema::CXXEqualityOperator:
+  case Sema::CXXInequalityOperator:
+    // FIXME:Oleg:
+    //  - do I need to check whether we have built-ins vs overloaded calls?
+    return Sema::ImplicitExceptionSpecification(S);
   case Sema::CXXInvalid:
     break;
   }
@@ -4791,7 +4799,10 @@ void Sema::CheckExplicitlyDefaultedSpecialMember(CXXMethodDecl *MD) {
     CanHaveConstParam = RD->implicitCopyConstructorHasConstParam();
   else if (CSM == CXXCopyAssignment)
     CanHaveConstParam = RD->implicitCopyAssignmentHasConstParam();
+  else if (CSM == CXXEqualityOperator || CSM == CXXInequalityOperator)
+    CanHaveConstParam = true;
 
+  // Verify assignment operator declaration
   QualType ReturnType = Context.VoidTy;
   if (CSM == CXXCopyAssignment || CSM == CXXMoveAssignment) {
     // Check for return type matching.
@@ -4812,6 +4823,25 @@ void Sema::CheckExplicitlyDefaultedSpecialMember(CXXMethodDecl *MD) {
     }
   }
 
+  // Verify equality operator declaration
+  if (CSM == CXXEqualityOperator || CSM == CXXInequalityOperator) {
+    // Check the return type
+    if (!Context.hasSameType(Type->getResultType(), Context.BoolTy)) {
+      Diag(MD->getLocation(), diag::err_defaulted_comp_member_return_type)
+        << "equality" << Context.BoolTy;
+      HadError = true;
+    }
+
+    // A defaulted special member needs to be "const"
+#if 0 // FIXME:Oleg:
+    if (Type->getTypeQuals()) {
+      Diag(MD->getLocation(), diag::err_defaulted_special_member_quals)
+        << true << getLangOpts().CPlusPlus1y;
+      HadError = true;
+    }
+#endif
+  }
+
   // Check for parameter type matching.
   QualType ArgType = ExpectedParams ? Type->getArgType(0) : QualType();
   bool HasConstParam = false;
@@ -4832,12 +4862,15 @@ void Sema::CheckExplicitlyDefaultedSpecialMember(CXXMethodDecl *MD) {
              diag::err_defaulted_special_member_copy_const_param)
           << (CSM == CXXCopyAssignment);
         // FIXME: Explain why this special member can't be const.
+        HadError = true;
+      } else if (CSM == CXXEqualityOperator || CSM == CXXInequalityOperator) {
+        assert(false && "Const is OK on params, so this should never execute");
       } else {
         Diag(MD->getLocation(),
              diag::err_defaulted_special_member_move_const_param)
           << (CSM == CXXMoveAssignment);
+        HadError = true;
       }
-      HadError = true;
     }
   } else if (ExpectedParams) {
     // A copy assignment operator can take its argument by value, but a
@@ -4883,7 +4916,11 @@ void Sema::CheckExplicitlyDefaultedSpecialMember(CXXMethodDecl *MD) {
   }
 
   //   If a function is explicitly defaulted on its first declaration,
-  if (First) {
+  // FIXME:Oleg:
+  //  - the point here is that the return type of default (in)equality members 
+  //    must not be mutated
+  //  - yet, is it correct to boardly hack this out?
+  if (First && CSM != CXXEqualityOperator && CSM != CXXInequalityOperator) {
     //  -- it is implicitly considered to be constexpr if the implicit
     //     definition would be,
     MD->setConstexpr(Constexpr);
@@ -4996,6 +5033,10 @@ struct SpecialMemberDeletionInfo {
         IsAssignment = true;
         IsMove = true;
         break;
+      case Sema::CXXEqualityOperator:
+      case Sema::CXXInequalityOperator:
+        ConstArg = true;
+        break;
       case Sema::CXXDestructor:
         break;
       case Sema::CXXInvalid:
@@ -5415,6 +5456,10 @@ static bool findTrivialSpecialMember(Sema &S, CXXRecordDecl *RD,
   case Sema::CXXInvalid:
     llvm_unreachable("not a special member");
 
+  case Sema::CXXEqualityOperator:
+  case Sema::CXXInequalityOperator:
+    return false;
+
   case Sema::CXXDefaultConstructor:
     // C++11 [class.ctor]p5:
     //   A default constructor is trivial if:
@@ -5714,6 +5759,10 @@ bool Sema::SpecialMemberIsTrivial(CXXMethodDecl *MD, CXXSpecialMember CSM,
     break;
   }
 
+  case CXXEqualityOperator:
+  case CXXInequalityOperator:
+    return false;
+
   case CXXInvalid:
     llvm_unreachable("not a special member");
   }
@@ -9600,6 +9649,253 @@ void Sema::DefineImplicitCopyAssignment(SourceLocation CurrentLocation,
   }
 }
 
+static ExprResult
+buildSingleEqualityCheck(Sema &S, SourceLocation Loc, QualType T,
+                         const ExprBuilder &LHS, const ExprBuilder &RHS) {
+  // Handle builtins first
+  if (T->isBuiltinType())
+    return S.CreateBuiltinBinOp(Loc, BO_EQ, LHS.build(S, Loc), RHS.build(S, Loc));
+
+  const RecordType *RecordTy = T->getAs<RecordType>();
+  CXXRecordDecl *ClassDecl = cast<CXXRecordDecl>(RecordTy->getDecl());
+
+  // Look for operator==()
+  DeclarationName Name = S.Context.DeclarationNames.getCXXOperatorName(OO_EqualEqual);
+  LookupResult OpLookup(S, Name, Loc, Sema::LookupOrdinaryName);
+  S.LookupQualifiedName(OpLookup, ClassDecl, false);
+
+  // Create the nested-name-specifier that will be used to qualify the
+  // reference to operator=; this is required to suppress the virtual
+  // call mechanism.
+  CXXScopeSpec SS;
+  const Type *CanonicalT = S.Context.getCanonicalType(T.getTypePtr());
+  SS.MakeTrivial(S.Context,
+                 NestedNameSpecifier::Create(S.Context, 0, false,
+                                             CanonicalT),
+                 Loc);
+
+  // Create the reference to the LHS' operator==() member function
+  ExprResult OpEqualRef
+    = S.BuildMemberReferenceExpr(LHS.build(S, Loc), T, Loc, /*isArrow=*/false,
+                                 SS, /*TemplateKWLoc=*/SourceLocation(),
+                                 /*FirstQualifierInScope=*/0,
+                                 OpLookup,
+                                 /*TemplateArgs=*/0,
+                                 /*SuppressQualifierCheck=*/true);
+  if (OpEqualRef.isInvalid())
+    return ExprError();
+
+  // 
+  // Finally build the call to the equality operator
+  //
+  Expr *Arg = RHS.build(S, Loc);
+  ExprResult Call = S.BuildCallToMemberFunction(/*Scope=*/0,
+                                                OpEqualRef.takeAs<Expr>(),
+                                                Loc, Arg, Loc);
+  if (Call.isInvalid())
+    return ExprError();
+
+  return Call;
+}
+
+void Sema::DefineDefaultEquality(SourceLocation CurrentLocation,
+                                 CXXMethodDecl *EqualityOperator) {
+  assert((EqualityOperator->isDefaulted() && 
+          EqualityOperator->isOverloadedOperator() &&
+          EqualityOperator->getOverloadedOperator() == OO_EqualEqual
+          /* &&
+          !EqualityOperator->doesThisDeclarationHaveABody() &&
+          !EqualityOperator->isDeleted()*/) &&
+         "DefineDefaultEquality called for wrong function");
+
+  CXXRecordDecl *ClassDecl = EqualityOperator->getParent();
+
+  if (ClassDecl->isInvalidDecl() || EqualityOperator->isInvalidDecl()) {
+    EqualityOperator->setInvalidDecl();
+    return;
+  }
+
+  SynthesizedFunctionScope Scope(*this, EqualityOperator);
+  DiagnosticErrorTrap Trap(Diags);
+
+  // The statements that form the synthesized function body.
+  SmallVector<Stmt*, 8> Statements;
+  
+  // The parameter for the "other" object, which we are copying from.
+  ParmVarDecl *Other = EqualityOperator->getParamDecl(0);
+  Qualifiers OtherQuals = Other->getType().getQualifiers();
+  QualType OtherRefType = Other->getType();
+  if (const LValueReferenceType *OtherRef
+                                = OtherRefType->getAs<LValueReferenceType>()) {
+    OtherRefType = OtherRef->getPointeeType();
+    OtherQuals = OtherRefType.getQualifiers();
+  }
+  
+  // Our location for everything implicitly-generated.
+  SourceLocation Loc = EqualityOperator->getLocation();
+  
+  // Builds a DeclRefExpr for the "other" object.
+  RefBuilder OtherRef(Other, OtherRefType);
+
+  // Builds the "this" pointer.
+  ThisBuilder This;
+  
+  // Compare each non-static member
+  bool Invalid = false;
+  for (CXXRecordDecl::field_iterator Field = ClassDecl->field_begin(),
+                                  FieldEnd = ClassDecl->field_end(); 
+       Field != FieldEnd; ++Field) {
+    if (Field->isUnnamedBitfield())
+      continue;
+
+    if (Field->isInvalidDecl()) {
+      Invalid = true;
+      continue;
+    }
+
+    // Suppress comparing zero-width bitfields.
+    if (Field->isBitField() && Field->getBitWidthValue(Context) == 0)
+      continue;
+    
+    QualType FieldType = Field->getType().getNonReferenceType();
+    if (FieldType->isIncompleteArrayType()) {
+      assert(ClassDecl->hasFlexibleArrayMember() && 
+             "Incomplete array type is not valid");
+      continue;
+    }
+    
+    // Build references to the field in the object we're copying from and to.
+    LookupResult MemberLookup(*this, Field->getDeclName(), Loc,
+                              LookupMemberName);
+    MemberLookup.addDecl(*Field);
+    MemberLookup.resolveKind();
+
+    MemberBuilder RHS(OtherRef, OtherRefType, /*IsArrow=*/false, MemberLookup);
+    MemberBuilder LHS(This, getCurrentThisType(), /*IsArrow=*/true, MemberLookup);
+
+    // Build member comparison
+    ExprResult Compare = buildSingleEqualityCheck(*this, Loc, FieldType, LHS, RHS);
+    if (Compare.isInvalid()) {
+      EqualityOperator->setInvalidDecl();
+      return;
+    }
+
+    // The "return false" bit
+    ExprResult Val = ActOnIntegerConstant(Loc, 0);
+    StmtResult Return = ActOnReturnStmt(Loc, Val.get());
+
+    // The whole conditional expr
+    StmtResult If = Owned(new (Context) IfStmt(Context, Loc, 0, Compare.get(), Return.get()));
+    if (If.isInvalid()) {
+      Diag(CurrentLocation, diag::note_member_synthesized_at) 
+        << CXXEqualityOperator << Context.getTagDeclType(ClassDecl);
+      EqualityOperator->setInvalidDecl();
+      return;
+    }      
+
+    // Success! Record the single member's check
+    Statements.push_back(If.takeAs<Stmt>());
+  }
+
+  if (!Invalid) {
+    // Add a "return true;" to the end of the method
+    ExprResult Val = ActOnIntegerConstant(Loc, 1);
+    StmtResult Return = ActOnReturnStmt(Loc, Val.get());
+    if (Return.isInvalid()) {
+      Diag(CurrentLocation, diag::note_member_synthesized_at) 
+        << CXXEqualityOperator << Context.getTagDeclType(ClassDecl);
+      EqualityOperator->setInvalidDecl();
+      return;
+    }      
+    Statements.push_back(Return.takeAs<Stmt>());
+  }
+
+  if (Invalid) {
+    EqualityOperator->setInvalidDecl();
+    return;
+  }
+
+  StmtResult Body;
+  {
+    CompoundScopeRAII CompoundScope(*this);
+    Body = ActOnCompoundStmt(Loc, Loc, Statements,
+                             /*isStmtExpr=*/false);
+    assert(!Body.isInvalid() && "Compound statement creation cannot fail");
+  }
+  EqualityOperator->setBody(Body.takeAs<Stmt>());
+
+  if (ASTMutationListener *L = getASTMutationListener()) {
+    L->CompletedImplicitDefinition(EqualityOperator);
+  }
+}
+
+void Sema::DefineDefaultInequality(SourceLocation CurrentLocation,
+                                   CXXMethodDecl *InequalityOperator) {
+  assert((InequalityOperator->isDefaulted() && 
+          InequalityOperator->isOverloadedOperator() &&
+          InequalityOperator->getOverloadedOperator() == OO_ExclaimEqual
+          /* &&
+          !InequalityOperator->doesThisDeclarationHaveABody() &&
+          !InequalityOperator->isDeleted()*/) &&
+         "DefineDefaultInequality called for wrong function");
+
+  CXXRecordDecl *ClassDecl = InequalityOperator->getParent();
+  QualType ClassType = Context.getTypeDeclType(ClassDecl);
+
+  if (ClassDecl->isInvalidDecl() || InequalityOperator->isInvalidDecl()) {
+    InequalityOperator->setInvalidDecl();
+    return;
+  }
+
+  SynthesizedFunctionScope Scope(*this, InequalityOperator);
+  DiagnosticErrorTrap Trap(Diags);
+
+  // The parameter for the "other" object, which we are copying from.
+  ParmVarDecl *Other = InequalityOperator->getParamDecl(0);
+  Qualifiers OtherQuals = Other->getType().getQualifiers();
+  QualType OtherRefType = Other->getType();
+  if (const LValueReferenceType *OtherRef
+                                = OtherRefType->getAs<LValueReferenceType>()) {
+    OtherRefType = OtherRef->getPointeeType();
+    OtherQuals = OtherRefType.getQualifiers();
+  }
+  
+  // Our location for everything implicitly-generated.
+  SourceLocation Loc = InequalityOperator->getLocation();
+  
+  // Get "this" and "param" refs
+  RefBuilder OtherRef(Other, OtherRefType);
+  ThisBuilder This;
+  
+  // Build the following expression (as I know it is a composite one):
+  //   (*this).operator==(param)
+  ExprResult Compare = buildSingleEqualityCheck(*this, Loc, ClassType, DerefBuilder(This), OtherRef);
+  if (Compare.isInvalid()) {
+    InequalityOperator->setInvalidDecl();
+    return;
+  }
+
+  // Build the negation bit
+  ExprResult Negation = CreateBuiltinUnaryOp(Loc, UO_LNot, Compare.get());
+  if (Negation.isInvalid()) {
+    InequalityOperator->setInvalidDecl();
+    return;
+  }
+
+  // Build the return statement
+  StmtResult Return = ActOnReturnStmt(Loc, Negation.get());
+  if (Return.isInvalid()) {
+    InequalityOperator->setInvalidDecl();
+    return;
+  }
+ 
+  InequalityOperator->setBody(Return.takeAs<Stmt>());
+
+  if (ASTMutationListener *L = getASTMutationListener()) {
+    L->CompletedImplicitDefinition(InequalityOperator);
+  }
+}
+
 Sema::ImplicitExceptionSpecification
 Sema::ComputeDefaultedMoveAssignmentExceptionSpec(CXXMethodDecl *MD) {
   CXXRecordDecl *ClassDecl = MD->getParent();
@@ -11995,8 +12291,15 @@ void Sema::SetDeclDefaulted(Decl *Dcl, SourceLocation DefaultLoc) {
     // If the method was defaulted on its first declaration, we will have
     // already performed the checking in CheckCompletedCXXClass. Such a
     // declaration doesn't trigger an implicit definition.
-    if (Primary == Primary->getCanonicalDecl())
+
+    // FIXME:Oleg:
+    //  - I have no idea what this means, yet I need to generate the code
+    //
+    if (Primary == Primary->getCanonicalDecl() && 
+      /* hack */ Member != CXXEqualityOperator &&
+                 Member != CXXInequalityOperator) {
       return;
+    }
 
     CheckExplicitlyDefaultedSpecialMember(MD);
 
@@ -12028,6 +12331,12 @@ void Sema::SetDeclDefaulted(Decl *Dcl, SourceLocation DefaultLoc) {
     case CXXMoveAssignment:
       DefineImplicitMoveAssignment(DefaultLoc, MD);
       break;
+    case CXXEqualityOperator:
+      DefineDefaultEquality(DefaultLoc, MD);
+      break;
+    case CXXInequalityOperator:
+      DefineDefaultInequality(DefaultLoc, MD);
+      break;
     case CXXInvalid:
       llvm_unreachable("Invalid special member.");
     }


More information about the cfe-dev mailing list