ISTM that there's no established way to tell Sema::Diag to not emit its diagnostic, and trying to swap to e.g. Sema::CanPerformCopyInitialization makes us too forgiving. So, given that we need to use CheckSingleAssignmentConstraints directly, I don't see a better way to handle this than plumbing the `Diagnose` boolean through.
I'll take a pass at fixing it, and hopefully get a patch out tomorrow. On Tue, Jan 12, 2016 at 1:51 PM, George Burgess IV < george.burgess...@gmail.com> wrote: > Sorry for the delayed response; one of my filters silently marked this > mail as read. Looking into it now. :) > > On Fri, Jan 8, 2016 at 12:38 PM, Bob Wilson <bob.wil...@apple.com> wrote: > >> George, >> >> This change caused a serious regression for Objective-C method lookup. >> See PR26085 (http://llvm.org/pr26085). >> >> For the test case in that PR, Sema::SelectBestMethod looks at the two >> candidate "test" methods. It will match the second one, but in the process >> of considering the first candidate, an error diagnostic is generated. This >> happens within the call to CheckSingleAssignmentConstraints that was added >> here in IsStandardConversion. The "Diagnose" argument in that call is set >> to false, but the diagnostic is generated anyway. For the test case in the >> PR, the diagnostic comes from CheckObjCARCConversion, but it looks like >> there are some other diagnostics that could also be generated from within >> CheckSingleAssignmentConstraints. >> >> I think I could manage a fix, e.g., by threading the “Diagnose” flag >> through all the relevant code and consistently checking it before emitting >> diagnostics, but I’m not especially familiar with this part of clang. If >> you or someone else who knows more about this area can figure out the best >> way to fix it, I would appreciate it. >> >> —Bob >> >> > On Oct 11, 2015, at 1:13 PM, George Burgess IV via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> > >> > Author: gbiv >> > Date: Sun Oct 11 15:13:20 2015 >> > New Revision: 249995 >> > >> > URL: http://llvm.org/viewvc/llvm-project?rev=249995&view=rev >> > Log: >> > [Sema] Allow C conversions in C overload logic >> > >> > C allows for some implicit conversions that C++ does not, e.g. void* -> >> > char*. This patch teaches clang that these conversions are okay when >> > dealing with overloads in C. >> > >> > Differential Revision: http://reviews.llvm.org/D13604 >> > >> > Modified: >> > cfe/trunk/include/clang/Sema/Overload.h >> > cfe/trunk/include/clang/Sema/Sema.h >> > cfe/trunk/lib/Sema/SemaExpr.cpp >> > cfe/trunk/lib/Sema/SemaOverload.cpp >> > cfe/trunk/test/Sema/overloadable.c >> > >> > Modified: cfe/trunk/include/clang/Sema/Overload.h >> > URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Overload.h?rev=249995&r1=249994&r2=249995&view=diff >> > >> ============================================================================== >> > --- cfe/trunk/include/clang/Sema/Overload.h (original) >> > +++ cfe/trunk/include/clang/Sema/Overload.h Sun Oct 11 15:13:20 2015 >> > @@ -83,7 +83,8 @@ namespace clang { >> > ICK_TransparentUnionConversion, ///< Transparent Union Conversions >> > ICK_Writeback_Conversion, ///< Objective-C ARC writeback conversion >> > ICK_Zero_Event_Conversion, ///< Zero constant to event (OpenCL1.2 >> 6.12.10) >> > - ICK_Num_Conversion_Kinds ///< The number of conversion kinds >> > + ICK_C_Only_Conversion, ///< Conversions allowed in C, but not >> C++ >> > + ICK_Num_Conversion_Kinds, ///< The number of conversion kinds >> > }; >> > >> > /// ImplicitConversionRank - The rank of an implicit conversion >> > @@ -95,7 +96,9 @@ namespace clang { >> > ICR_Promotion, ///< Promotion >> > ICR_Conversion, ///< Conversion >> > ICR_Complex_Real_Conversion, ///< Complex <-> Real conversion >> > - ICR_Writeback_Conversion ///< ObjC ARC writeback conversion >> > + ICR_Writeback_Conversion, ///< ObjC ARC writeback conversion >> > + ICR_C_Conversion ///< Conversion only allowed in the C >> standard. >> > + /// (e.g. void* to char*) >> > }; >> > >> > ImplicitConversionRank GetConversionRank(ImplicitConversionKind Kind); >> > >> > Modified: cfe/trunk/include/clang/Sema/Sema.h >> > URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=249995&r1=249994&r2=249995&view=diff >> > >> ============================================================================== >> > --- cfe/trunk/include/clang/Sema/Sema.h (original) >> > +++ cfe/trunk/include/clang/Sema/Sema.h Sun Oct 11 15:13:20 2015 >> > @@ -8292,19 +8292,23 @@ public: >> > QualType LHSType, >> > QualType RHSType); >> > >> > - /// Check assignment constraints and prepare for a conversion of the >> > - /// RHS to the LHS type. >> > + /// Check assignment constraints and optionally prepare for a >> conversion of >> > + /// the RHS to the LHS type. The conversion is prepared for if >> ConvertRHS >> > + /// is true. >> > AssignConvertType CheckAssignmentConstraints(QualType LHSType, >> > ExprResult &RHS, >> > - CastKind &Kind); >> > + CastKind &Kind, >> > + bool ConvertRHS = true); >> > >> > // CheckSingleAssignmentConstraints - Currently used by >> > // CheckAssignmentOperands, and ActOnReturnStmt. Prior to type >> checking, >> > - // this routine performs the default function/array converions. >> > + // this routine performs the default function/array converions, if >> ConvertRHS >> > + // is true. >> > AssignConvertType CheckSingleAssignmentConstraints(QualType LHSType, >> > ExprResult &RHS, >> > bool Diagnose = >> true, >> > - bool >> DiagnoseCFAudited = false); >> > + bool >> DiagnoseCFAudited = false, >> > + bool ConvertRHS = >> true); >> > >> > // \brief If the lhs type is a transparent union, check whether we >> > // can initialize the transparent union with the given expression. >> > >> > Modified: cfe/trunk/lib/Sema/SemaExpr.cpp >> > URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=249995&r1=249994&r2=249995&view=diff >> > >> ============================================================================== >> > --- cfe/trunk/lib/Sema/SemaExpr.cpp (original) >> > +++ cfe/trunk/lib/Sema/SemaExpr.cpp Sun Oct 11 15:13:20 2015 >> > @@ -5376,13 +5376,13 @@ CastKind Sema::PrepareScalarCast(ExprRes >> > return CK_IntegralToFloating; >> > case Type::STK_IntegralComplex: >> > Src = ImpCastExprToType(Src.get(), >> > - >> DestTy->castAs<ComplexType>()->getElementType(), >> > - CK_IntegralCast); >> > + DestTy->castAs<ComplexType>()->getElementType(), >> > + CK_IntegralCast); >> > return CK_IntegralRealToComplex; >> > case Type::STK_FloatingComplex: >> > Src = ImpCastExprToType(Src.get(), >> > - >> DestTy->castAs<ComplexType>()->getElementType(), >> > - CK_IntegralToFloating); >> > + DestTy->castAs<ComplexType>()->getElementType(), >> > + CK_IntegralToFloating); >> > return CK_FloatingRealToComplex; >> > case Type::STK_MemberPointer: >> > llvm_unreachable("member pointer type in C"); >> > @@ -6867,7 +6867,7 @@ Sema::CheckAssignmentConstraints(SourceL >> > ExprResult RHSPtr = &RHSExpr; >> > CastKind K = CK_Invalid; >> > >> > - return CheckAssignmentConstraints(LHSType, RHSPtr, K); >> > + return CheckAssignmentConstraints(LHSType, RHSPtr, K, >> /*ConvertRHS=*/false); >> > } >> > >> > /// CheckAssignmentConstraints (C99 6.5.16) - This routine currently >> > @@ -6889,7 +6889,7 @@ Sema::CheckAssignmentConstraints(SourceL >> > /// Sets 'Kind' for any result kind except Incompatible. >> > Sema::AssignConvertType >> > Sema::CheckAssignmentConstraints(QualType LHSType, ExprResult &RHS, >> > - CastKind &Kind) { >> > + CastKind &Kind, bool ConvertRHS) { >> > QualType RHSType = RHS.get()->getType(); >> > QualType OrigLHSType = LHSType; >> > >> > @@ -6911,7 +6911,7 @@ Sema::CheckAssignmentConstraints(QualTyp >> > CheckAssignmentConstraints(AtomicTy->getValueType(), RHS, Kind); >> > if (result != Compatible) >> > return result; >> > - if (Kind != CK_NoOp) >> > + if (Kind != CK_NoOp && ConvertRHS) >> > RHS = ImpCastExprToType(RHS.get(), AtomicTy->getValueType(), >> Kind); >> > Kind = CK_NonAtomicToAtomic; >> > return Compatible; >> > @@ -6941,7 +6941,7 @@ Sema::CheckAssignmentConstraints(QualTyp >> > // CK_VectorSplat does T -> vector T, so first cast to the >> > // element type. >> > QualType elType = cast<ExtVectorType>(LHSType)->getElementType(); >> > - if (elType != RHSType) { >> > + if (elType != RHSType && ConvertRHS) { >> > Kind = PrepareScalarCast(RHS, elType); >> > RHS = ImpCastExprToType(RHS.get(), elType, Kind); >> > } >> > @@ -6974,7 +6974,8 @@ Sema::CheckAssignmentConstraints(QualTyp >> > // Arithmetic conversions. >> > if (LHSType->isArithmeticType() && RHSType->isArithmeticType() && >> > !(getLangOpts().CPlusPlus && LHSType->isEnumeralType())) { >> > - Kind = PrepareScalarCast(RHS, LHSType); >> > + if (ConvertRHS) >> > + Kind = PrepareScalarCast(RHS, LHSType); >> > return Compatible; >> > } >> > >> > @@ -7099,7 +7100,8 @@ Sema::CheckAssignmentConstraints(QualTyp >> > // Only under strict condition T^ is compatible with an Objective-C >> pointer. >> > if (RHSType->isBlockPointerType() && >> > LHSType->isBlockCompatibleObjCPointerType(Context)) { >> > - maybeExtendBlockObject(RHS); >> > + if (ConvertRHS) >> > + maybeExtendBlockObject(RHS); >> > Kind = CK_BlockPointerToObjCPointerCast; >> > return Compatible; >> > } >> > @@ -7225,9 +7227,16 @@ Sema::CheckTransparentUnionArgumentConst >> > } >> > >> > Sema::AssignConvertType >> > -Sema::CheckSingleAssignmentConstraints(QualType LHSType, ExprResult >> &RHS, >> > +Sema::CheckSingleAssignmentConstraints(QualType LHSType, ExprResult >> &CallerRHS, >> > bool Diagnose, >> > - bool DiagnoseCFAudited) { >> > + bool DiagnoseCFAudited, >> > + bool ConvertRHS) { >> > + // If ConvertRHS is false, we want to leave the caller's RHS >> untouched. Sadly, >> > + // we can't avoid *all* modifications at the moment, so we need some >> somewhere >> > + // to put the updated value. >> > + ExprResult LocalRHS = CallerRHS; >> > + ExprResult &RHS = ConvertRHS ? CallerRHS : LocalRHS; >> > + >> > if (getLangOpts().CPlusPlus) { >> > if (!LHSType->isRecordType() && !LHSType->isAtomicType()) { >> > // C++ 5.17p3: If the left operand is not of class type, the >> > @@ -7276,7 +7285,8 @@ Sema::CheckSingleAssignmentConstraints(Q >> > CastKind Kind; >> > CXXCastPath Path; >> > CheckPointerConversion(RHS.get(), LHSType, Kind, Path, false); >> > - RHS = ImpCastExprToType(RHS.get(), LHSType, Kind, VK_RValue, >> &Path); >> > + if (ConvertRHS) >> > + RHS = ImpCastExprToType(RHS.get(), LHSType, Kind, VK_RValue, >> &Path); >> > return Compatible; >> > } >> > >> > @@ -7287,6 +7297,7 @@ Sema::CheckSingleAssignmentConstraints(Q >> > // >> > // Suppress this for references: C++ 8.5.3p5. >> > if (!LHSType->isReferenceType()) { >> > + // FIXME: We potentially allocate here even if ConvertRHS is false. >> > RHS = DefaultFunctionArrayLvalueConversion(RHS.get()); >> > if (RHS.isInvalid()) >> > return Incompatible; >> > @@ -7303,7 +7314,7 @@ Sema::CheckSingleAssignmentConstraints(Q >> > >> > CastKind Kind = CK_Invalid; >> > Sema::AssignConvertType result = >> > - CheckAssignmentConstraints(LHSType, RHS, Kind); >> > + CheckAssignmentConstraints(LHSType, RHS, Kind, ConvertRHS); >> > >> > // C99 6.5.16.1p2: The value of the right operand is converted to the >> > // type of the assignment expression. >> > @@ -7325,7 +7336,8 @@ Sema::CheckSingleAssignmentConstraints(Q >> > return Compatible; >> > } >> > >> > - RHS = ImpCastExprToType(E, Ty, Kind); >> > + if (ConvertRHS) >> > + RHS = ImpCastExprToType(E, Ty, Kind); >> > } >> > return result; >> > } >> > >> > Modified: cfe/trunk/lib/Sema/SemaOverload.cpp >> > URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=249995&r1=249994&r2=249995&view=diff >> > >> ============================================================================== >> > --- cfe/trunk/lib/Sema/SemaOverload.cpp (original) >> > +++ cfe/trunk/lib/Sema/SemaOverload.cpp Sun Oct 11 15:13:20 2015 >> > @@ -130,7 +130,11 @@ ImplicitConversionRank clang::GetConvers >> > ICR_Complex_Real_Conversion, >> > ICR_Conversion, >> > ICR_Conversion, >> > - ICR_Writeback_Conversion >> > + ICR_Writeback_Conversion, >> > + ICR_Exact_Match, // NOTE(gbiv): This may not be completely right -- >> > + // it was omitted by the patch that added >> > + // ICK_Zero_Event_Conversion >> > + ICR_C_Conversion >> > }; >> > return Rank[(int)Kind]; >> > } >> > @@ -162,7 +166,9 @@ static const char* GetImplicitConversion >> > "Complex-real conversion", >> > "Block Pointer conversion", >> > "Transparent Union Conversion", >> > - "Writeback conversion" >> > + "Writeback conversion", >> > + "OpenCL Zero Event Conversion", >> > + "C specific type conversion" >> > }; >> > return Name[Kind]; >> > } >> > @@ -1411,7 +1417,7 @@ static bool tryAtomicConversion(Sema &S, >> > bool InOverloadResolution, >> > StandardConversionSequence &SCS, >> > bool CStyle); >> > - >> > + >> > /// IsStandardConversion - Determines whether there is a standard >> > /// conversion sequence (C++ [conv], C++ [over.ics.scs]) from the >> > /// expression From to the type ToType. Standard conversion sequences >> > @@ -1434,13 +1440,10 @@ static bool IsStandardConversion(Sema &S >> > SCS.CopyConstructor = nullptr; >> > >> > // There are no standard conversions for class types in C++, so >> > - // abort early. When overloading in C, however, we do permit >> > - if (FromType->isRecordType() || ToType->isRecordType()) { >> > - if (S.getLangOpts().CPlusPlus) >> > - return false; >> > - >> > - // When we're overloading in C, we allow, as standard conversions, >> > - } >> > + // abort early. When overloading in C, however, we do permit them. >> > + if (S.getLangOpts().CPlusPlus && >> > + (FromType->isRecordType() || ToType->isRecordType())) >> > + return false; >> > >> > // The first conversion can be an lvalue-to-rvalue conversion, >> > // array-to-pointer conversion, or function-to-pointer conversion >> > @@ -1649,9 +1652,9 @@ static bool IsStandardConversion(Sema &S >> > // tryAtomicConversion has updated the standard conversion sequence >> > // appropriately. >> > return true; >> > - } else if (ToType->isEventT() && >> > + } else if (ToType->isEventT() && >> > From->isIntegerConstantExpr(S.getASTContext()) && >> > - (From->EvaluateKnownConstInt(S.getASTContext()) == 0)) { >> > + From->EvaluateKnownConstInt(S.getASTContext()) == 0) { >> > SCS.Second = ICK_Zero_Event_Conversion; >> > FromType = ToType; >> > } else { >> > @@ -1690,11 +1693,28 @@ static bool IsStandardConversion(Sema &S >> > } >> > SCS.setToType(2, FromType); >> > >> > + if (CanonFrom == CanonTo) >> > + return true; >> > + >> > // If we have not converted the argument type to the parameter type, >> > - // this is a bad conversion sequence. >> > - if (CanonFrom != CanonTo) >> > + // this is a bad conversion sequence, unless we're resolving an >> overload in C. >> > + if (S.getLangOpts().CPlusPlus || !InOverloadResolution) >> > return false; >> > >> > + ExprResult ER = ExprResult{From}; >> > + auto Conv = S.CheckSingleAssignmentConstraints(ToType, ER, >> > + /*Diagnose=*/false, >> > + >> /*DiagnoseCFAudited=*/false, >> > + /*ConvertRHS=*/false); >> > + if (Conv != Sema::Compatible) >> > + return false; >> > + >> > + SCS.setAllToTypes(ToType); >> > + // We need to set all three because we want this conversion to rank >> terribly, >> > + // and we don't know what conversions it may overlap with. >> > + SCS.First = ICK_C_Only_Conversion; >> > + SCS.Second = ICK_C_Only_Conversion; >> > + SCS.Third = ICK_C_Only_Conversion; >> > return true; >> > } >> > >> > @@ -4993,6 +5013,7 @@ static bool CheckConvertedConstantConver >> > case ICK_TransparentUnionConversion: >> > case ICK_Writeback_Conversion: >> > case ICK_Zero_Event_Conversion: >> > + case ICK_C_Only_Conversion: >> > return false; >> > >> > case ICK_Lvalue_To_Rvalue: >> > @@ -5785,7 +5806,7 @@ ObjCMethodDecl *Sema::SelectBestMethod(S >> > Match = false; >> > break; >> > } >> > - >> > + >> > ImplicitConversionSequence ConversionState >> > = TryCopyInitialization(*this, argExpr, param->getType(), >> > /*SuppressUserConversions*/false, >> > >> > Modified: cfe/trunk/test/Sema/overloadable.c >> > URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/overloadable.c?rev=249995&r1=249994&r2=249995&view=diff >> > >> ============================================================================== >> > --- cfe/trunk/test/Sema/overloadable.c (original) >> > +++ cfe/trunk/test/Sema/overloadable.c Sun Oct 11 15:13:20 2015 >> > @@ -85,3 +85,17 @@ void local() { >> > void after_local_1(int) __attribute__((overloadable)); // >> expected-error {{conflicting types}} >> > void after_local_2(int); // expected-error {{must have the >> 'overloadable' attribute}} >> > void after_local_3(int) __attribute__((overloadable)); >> > + >> > +// Make sure we allow C-specific conversions in C. >> > +void conversions() { >> > + void foo(char *c) __attribute__((overloadable)); >> > + void foo(char *c) __attribute__((overloadable, enable_if(c, >> "nope.jpg"))); >> > + >> > + void *ptr; >> > + foo(ptr); >> > + >> > + void multi_type(unsigned char *c) __attribute__((overloadable)); >> > + void multi_type(signed char *c) __attribute__((overloadable)); >> > + unsigned char *c; >> > + multi_type(c); >> > +} >> > >> > >> > _______________________________________________ >> > cfe-commits mailing list >> > cfe-commits@lists.llvm.org >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits