[cfe-commits] [PATCH] Multiple patches for refactoring SemaExpr.cpp

[Douglas Gregor]

On Aug 8, 2011, at 6:39 PM, Richard Trieu wrote:

> Refactoring of lib/Sema/SemaExpr.cpp broken into several patches.  Changes include reducing code duplication, moving diagnostic checks into separate functions, and reducing method length.  Comments welcome. <check_addition_subtraction_increment_decrement.patch>

Looks good.

> <check_address_of_operand.patch>

+/// \brief Diagnose invalid operand for address of operations.
+static void diagnoseAddressOfInvalidType(Sema &S, SourceLocation Loc,
+                                         Expr *E, std::string type) {
+  S.Diag(Loc, diag::err_typecheck_address_of) << type << E->getSourceRange();
+}

It would be far better for err_typecheck_address_of to use a %select{} in its diagnostic text, rather than passing strings like "bit-field" or "vector element" through to the diagnostics system. The current behavior isn't localizable at al.


> <check_conditional_operands.patch>

+/// \brief Return true if the condition expression is valid.
+static bool checkCondition(Sema &S, Expr *Cond) {
+  QualType CondTy = Cond->getType();
+
+  // C99 6.5.15p2
+  if (CondTy->isScalarType()) return true;
+
+  // OpenCL: Sec 6.3.i says the condition is allowed to be a vector or scalar.
+  if (S.getLangOptions().OpenCL && CondTy->isVectorType())
+    return true;
+
+  // Emit the proper error message.
+  S.Diag(Cond->getLocStart(), S.getLangOptions().OpenCL ?
+                              diag::err_typecheck_cond_expect_scalar :
+                              diag::err_typecheck_cond_expect_scalar_or_vector)
+    << CondTy;
+  return false;
+}

LLVM convention is to return true on failure, false otherwise. The same goes for the rest of the check* functions.

+/// \brief Checks compatibility between two pointers and return the resulting
+/// type.
+static QualType checkConditionalPointerCompatibility(Sema &S, ExprResult &LHS,
+                                                     ExprResult &RHS,
+                                                     SourceLocation Loc) {
+  QualType LHSTy = LHS.get()->getType();
+  QualType RHSTy = RHS.get()->getType();
+
+  if (S.Context.getCanonicalType(LHSTy) == S.Context.getCanonicalType(RHSTy)) {
+    // Two identical pointers types are always compatible.
+    return LHSTy;
+  }

It's a bit cleaner to write S.Context.hasSameType(LHSTy, RHSTy) here.

> <check_incomplete_pointer_type.patch>

+/// \brief Warn if Operand is incomplete pointer type
+///
+/// \returns True if pointer has complete type
+static bool checkArithmeticIncompletePointerType(Sema &S, SourceLocation Loc,
+                                                 Expr *Operand) {
+  if ((Operand->getType()->isPointerType() &&
+       !Operand->getType()->isDependentType()) ||
+      Operand->getType()->isObjCObjectPointerType()) {
+    QualType PointeeTy = Operand->getType()->getPointeeType();
+    if (S.RequireCompleteType(
+          Loc, PointeeTy,
+          S.PDiag(diag::err_typecheck_arithmetic_incomplete_type)
+            << PointeeTy << Operand->getSourceRange()))
+      return false;
+  }
+  return true;
+}

Please invert the return result.

> <pointer_comparison.patch>

+/// \brief Returns true if the member pointers are compatible.
+static bool checkPointerComparisonCompatibility(Sema &S, SourceLocation Loc,
+                                                ExprResult &lex,
+                                                ExprResult &rex) {

Return value inversion.


> <diagnose_bitwise_precedence.patch>

Looks good!

> <null_arithmetic_and_enum_compare_to_new_functions.patch>

+static void checkEnumComparison(Sema &S, SourceLocation Loc, ExprResult &lex,
+                                ExprResult &rex){
+  QualType lType = lex.get()->getType();
+  QualType rType = rex.get()->getType();
+  QualType LHSStrippedType = lex.get()->IgnoreParenImpCasts()->getType();
+  QualType RHSStrippedType = rex.get()->IgnoreParenImpCasts()->getType();
+
+  // Two different enums will raise a warning when compared.
+  if (const EnumType *LHSEnumType = LHSStrippedType->getAs<EnumType>()) {
+    if (const EnumType *RHSEnumType = RHSStrippedType->getAs<EnumType>()) {
+      if (LHSEnumType->getDecl()->getIdentifier() &&
+          RHSEnumType->getDecl()->getIdentifier() &&
+          !S.Context.hasSameUnqualifiedType(LHSStrippedType, RHSStrippedType)) {
+        S.Diag(Loc, diag::warn_comparison_of_mixed_enum_types)
+          << LHSStrippedType << RHSStrippedType
+          << lex.get()->getSourceRange() << rex.get()->getSourceRange();
+      }
+    }
+  }
+}

You could easily reduce nesting here to clean things up, e.g.,

  const EnumType *LHSEnumType = LHSStrippedType->getAs<EnumType>();
  if (!LHSEnumType)
    return;
  const EnumType *RHSEnumType = RHSStrippedType->getAs<EnumType>();
  if (!RHSEnumType)
    return;


> <usual_arithmetic_conversions.patch>

+/// \brief Converts an integer to complex float type.
+static bool handleIntegerToComplexFloatConversion(Sema &S, ExprResult &intExpr,
+                                                  ExprResult &complexExpr,
+                                                  QualType intTy,
+                                                  QualType complexTy,
+                                                  bool skipCast) {
+  if (intTy->isComplexType() || intTy->isRealFloatingType()) return false;
+  if (skipCast) return true;
+  if (intTy->isIntegerType()) {
+    QualType fpTy = cast<ComplexType>(complexTy)->getElementType();
+    intExpr = S.ImpCastExprToType(intExpr.take(), fpTy, CK_IntegralToFloating);
+    intExpr = S.ImpCastExprToType(intExpr.take(), complexTy,
+                                  CK_FloatingRealToComplex);
+  } else {
+    assert(intTy->isComplexIntegerType());
+    intExpr = S.ImpCastExprToType(intExpr.take(), complexTy,
+                                  CK_IntegralComplexToFloatingComplex);
   }
+  return true;
+}

Inverted return value.

These are good cleanups, but please make sure to follow the return-true-on-error convention used in LLVM/Clang. I think it's a weird convention, but it's pervasive :)

	- Doug