[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:16218 +checkNonTrivialCUnion(E->getType(), E->getExprLoc(), + Sema::NTCUC_LValueToRValueVolatile); + ahatanak wrote: > rjmccall wrote: > > ahatanak wrote: > > > rjmccal

[PATCH] D63975: Warn when ScopeDepthOrObjCQuals overflows

rjmccall added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:6587 +return; + } + Comment indentation. Should we do this when starting to parse a function prototype instead of when parsing a parameter? Repository: rG LLVM Github Mon

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:259 +return DefaultConstrainedRounding.getValue(); + } + Okay, so what are the invariants here now? It looks like, in order to enable constrained floating point, I need to also have se

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:259 +return DefaultConstrainedRounding.getValue(); + } + kpn wrote: > rjmccall wrote: > > Okay, so what are the invariants here now? It looks like, in order to > > enable constrained f

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:16218 +checkNonTrivialCUnion(E->getType(), E->getExprLoc(), + Sema::NTCUC_LValueToRValueVolatile); + ahatanak wrote: > rjmccall wrote: > > ahatanak wrote: > > > rjmccal

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1133 + /// Check if this is or contains a non-trivial C struct/union type. + bool hasNonTrivialPrimitiveCStruct() const; You only want these checks to trigger on unions with non-trivial memb

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

rjmccall added a comment. I agree with Eli that this isn't obviously a legal transformation. `llvm.ptrmask` appears to make semantic guarantees about e.g. the pointer after the mask referring to the same underlying object, which means we can only safely emit it when something about the source

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

rjmccall added a comment. Thanks, this looks good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53157/new/ https://reviews.llvm.org/D53157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1133 + /// Check if this is or contains a non-trivial C struct/union type. + bool hasNonTrivialPrimitiveCStruct() const; ahatanak wrote: > rjmccall wrote: > > You only want these checks to t

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

rjmccall added a comment. The pointer/integer conversion is "implementation-defined", but it's not totally unconstrained. C notes that "The mapping functions for converting a pointer to an integer or an integer to a pointer are intended to be consistent with the addressing structure of the exe

[PATCH] D62645: [Sema] Resolve placeholder types before type deduction to silence spurious `-Warc-repeated-use-of-weak` warnings

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62645/new/ https://reviews.llvm.org/D62645 ___ c

[PATCH] D62645: [Sema] Resolve placeholder types before type deduction to silence spurious `-Warc-repeated-use-of-weak` warnings

rjmccall added inline comments. Comment at: include/clang/Basic/Attr.td:1844 + let Documentation = [Undocumented]; +} + Oh, please add a comment on this explaining what it means, since it's not based on a language feature. Repository: rC Clang CHANGES SINC

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

rjmccall added a comment. In D64128#1569836 , @hfinkel wrote: > In D64128#1569817 , @rjmccall wrote: > > > The pointer/integer conversion is "implementation-defined", but it's not > > totally unconstrained. C note

[PATCH] D62960: Add SVE opaque built-in types

rjmccall added inline comments. Comment at: lib/AST/ItaniumMangle.cpp:2680 +break; +#include "clang/Basic/AArch64SVEACLETypes.def" } rsandifo-arm wrote: > rovka wrote: > > erik.pilkington wrote: > > > rsandifo-arm wrote: > > > > erik.pilkington wrote: > >

[PATCH] D53295: Mark store and load of block invoke function as invariant.group

rjmccall added a reviewer: ahatanak. rjmccall added a comment. Herald added a subscriber: dexonsmith. Okay. Akira, do you have any interest in looking into this as a general block optimization? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53295/new/ https://reviews.llvm.org/D53295

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

rjmccall added a comment. I would be opposed to any addition of a `-f` flag for this absent any evidence that it's valuable for some actual C code; this patch appears to be purely speculative. I certainly don't think we should be adding it default-on. Your argument about alignment is what I w

[PATCH] D63846: [clang] Preserve names of addrspacecast'ed values.

rjmccall added a comment. Please don't check IR names in test output. That actually includes anonymous names like `%2`; these should always be tested with FileCheck variables. I suggest using `%.*` as the pattern; if you're matching the LHS of an LLVM assignment, that shouldn't have problems

[PATCH] D60456: [RISCV] Hard float ABI support

rjmccall added a comment. I'm fine with proceeding with your best guess about what the ABI should be. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9232 +if (IsFloat && Size > FLen) + return false; +// Can't be eligible if an integer type was already found (only fp+

[PATCH] D60456: [RISCV] Hard float ABI support

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Yes, I think you can commit. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60456/new/ https://reviews.llvm.org/D60456 ___ cfe-commit

[PATCH] D63846: [clang] Preserve names of addrspacecast'ed values.

rjmccall added a comment. I don't know what I think about widespread use of `-fno-discard-value-names` for now; please continue to use FileCheck variables, and we can make a holistic decision about that flag later. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D6

[PATCH] D63856: [ObjC] Add a -Wtautological-compare warning for BOOL

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63856/new/ https://reviews.llvm.org/D63856 ___ cfe-commi

[PATCH] D63912: [ObjC] Add a warning for implicit conversions of a constant non-boolean value to BOOL

rjmccall added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3251 + "the only well defined values for BOOL are YES and NO">, + InGroup; The `from` seems awkward here; `%0` is the number, not the type. (I agree that the type i

[PATCH] D63846: [clang] Preserve names of addrspacecast'ed values.

rjmccall added a comment. In D63846#1574311 , @vzakhari wrote: > In D63846#1574302 , @rjmccall wrote: > > > I don't know what I think about widespread use of > > `-fno-discard-value-names` for now; please continue

[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors

rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.h:274 +return LangAS::Default; + } + This target hook should just return the address space of the `__cxa_atexit` argument, not to claim anything specific about the relation between that add

[PATCH] D63912: [ObjC] Add a warning for implicit conversions of a constant non-boolean value to BOOL

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3251 + "the only well defined values for BOOL are YES and NO">, + InGroup; erik.pilk

[PATCH] D63846: [clang] Preserve names of addrspacecast'ed values.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63846/new/ https://reviews.llvm.org/D63846 ___ cfe-commits mailing list cfe-co

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

rjmccall added a comment. I wouldn't favor adding something really obscure that was only useful for clang, but I think builtins to set and clear mask bits while promising to preserve object-reference identity would be more generally useful for libraries. For example, there might be somewhere i

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

rjmccall added a comment. Thanks, that looks great. A few more requests and then this will be ready to go, I think. Comment at: include/clang/AST/DeclBase.h:1454 /// Number of non-inherited bits in RecordDeclBitfields. enum { NumRecordDeclBits = 11 };

[PATCH] D64464: [CodeGen] Emit destructor calls for non-trivial C structs

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:4100 + if (E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) +pushDestroy(QualType::DK_nontrivial_c_struct, DeclPtr, E->getType()); + Unfortunately, the lifetime of compoun

[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

rjmccall added inline comments. Comment at: cfe/trunk/lib/Sema/SemaExprCXX.cpp:4229 +LangAS AddrSpaceR = +RHSType->getAs()->getPointeeType().getAddressSpace(); +CastKind Kind = All of this can be much simpler: ``` LangAS AddrSpaceL = ToType->cas

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

rjmccall added inline comments. Comment at: lib/Sema/TreeTransform.h:5363 +if (ResultType.getAddressSpace() != LangAS::Default && +(ResultType.getAddressSpace() != LangAS::opencl_private)) { SemaRef.Diag(TL.getReturnLoc().getBeginLoc(), Anastas

[PATCH] D64400: [OpenCL][PR42390] Deduce addr space for templ specialization

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:6512 +inline bool Type::isTemplateSpecializationType() const { + return isa(this); +} This is a sugar type. What are you trying to do? Comment at: lib/Sema/SemaType.cpp:7417

[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

rjmccall added inline comments. Comment at: cfe/trunk/lib/Sema/SemaExprCXX.cpp:4229 +LangAS AddrSpaceR = +RHSType->getAs()->getPointeeType().getAddressSpace(); +CastKind Kind = Anastasia wrote: > rjmccall wrote: > > All of this can be much simpler

[PATCH] D64400: [OpenCL][PR42390] Deduce addr space for templ specialization

rjmccall added inline comments. Comment at: lib/Sema/SemaType.cpp:7421 + // - template specialization as addr space in template argument doesn't + // affect specialization. + (T->isDependentType() && (!T->isPointerType() && !T->isReferenceType() && --

[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors

rjmccall added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); rjmccall wrote: > Anastasia wrote: > > rjmccal

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

rjmccall added a comment. I agree that tools shouldn't be forced to deal with invalid AST that looks like valid AST. To me that means finding ways to preserve information that (1) don't badly violate invariants and (2) are easily discoverable as invalid. For `case`, which has external require

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

rjmccall added a comment. I agree that we really shouldn't use most of those — we shouldn't have null types or null child expressions anywhere in the AST. (Accessors might return null for *optional* children, of course, but that's different.) And yeah, a big part of that would be having an er

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:6097 + // non-trivial to copy or default-initialize. + checkNonTrivialCUnionInInitList(ILE); + } ahatanak wrote: > rjmccall wrote: > > Can we extract a common function that checks the in

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:12053 +NTCUC_UninitAutoVar); } + ahatanak wrote: > rjmccall wrote: > > Please add a comment explaining why this is specific to local variables. > I was trying to ex

[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors

rjmccall added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); Anastasia wrote: > rjmccall wrote: > > rjmccal

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

rjmccall added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3915 +Here, if the construction of `array[9]` fails with an exception, `array[0..8]` +will be destroyed, so the element's destructor needs to be accessible. }]; Probably worth ad

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122 + if (VD->isNoDestroy(getASTContext()) && + !(VD->getType()->isArrayType() && getLangOpts().Exceptions && +VD->isStaticLocal())) return; erik.pilkington wrot

[PATCH] D61667: Assume `__cxa_allocate_exception` returns an under-aligned memory on Darwin if the version of libc++abi isn't new enough to include the fix in r319123

rjmccall added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:946 + // is larger than the minimum alignment the libc++abi runtime guarantees. + if (Context.getTargetInfo().getTriple().isOSDarwin()) { +CharUnits TypeAlign = Context.getTypeAlignInChars(Ty); ---

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/test/SemaCXX/aggregate-initialization.cpp:199 void test0() { -auto *y = new Y {}; // expected-error {{temporary of type 'ElementDestructor

[PATCH] D61667: Assume `__cxa_allocate_exception` returns an under-aligned memory on Darwin if the version of libc++abi isn't new enough to include the fix in r319123

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61667/new/ https://reviews.llvm.org/D61667 ___ c

[PATCH] D61788: Make getObjCEncodingForTypeImpl() take a bitmask instead of 8 bools

rjmccall added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:2877 +OCET_EncodePointerToObjCTypedef = 1 << 7, + }; + void getObjCEncodingForTypeImpl(QualType t, std::string &S, unsigned Options, I like the idea of doing this, but can you

[PATCH] D61788: Make getObjCEncodingForTypeImpl() take a bitmask instead of 8 bools

rjmccall added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:2877 +OCET_EncodePointerToObjCTypedef = 1 << 7, + }; + void getObjCEncodingForTypeImpl(QualType t, std::string &S, unsigned Options, thakis wrote: > rjmccall wrote: > > I like

[PATCH] D61970: [CodeGen][ObjC] Call objc_autoreleaseReturnValue and objc_retainAutoreleasedReturnValue instead of objc_autorelease and objc_retain in MRR

rjmccall added inline comments. Comment at: include/clang/Driver/Options.td:1500 + Flag<["-"], "fobjc-no-builtin-retain-release">, Group, + Flags<[CC1Option]>; def fno_objc_convert_messages_to_runtime_calls : I know I suggested this name, but it should probabl

[PATCH] D60456: [RISCV][WIP/RFC] Hard float ABI support

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9232 +if (IsFloat && Size > FLen) + return false; +// Can't be eligible if an integer type was already found (only fp+int or Is this the only consideration for floating-poin

[PATCH] D59628: Add support for __attribute__((objc_class_stub))

rjmccall added a comment. Thanks! Down to minor stuff now. Comment at: clang/include/clang/Basic/Attr.td:291 + string CustomCode = customCode; } def MicrosoftExt : LangOpt<"MicrosoftExt">; Please add a comment here explaining how to use `CustomCode`. Can w

[PATCH] D62156: [Sema] Diagnose addr space mismatch while constructing objects

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:8229 + if (FTI.hasMethodTypeCVRUQualifiers()) { +FTI.MethodQualifiers->forEachCVRUQualifier( [&](DeclSpec::TQ TypeQual, StringRef QualName, SourceLocation SL) { We want to catch `_

[PATCH] D59628: Add support for __attribute__((objc_class_stub))

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, thanks! You should wait for Aaron's approval, too. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59628/new/ https://reviews.llvm.org/D59628 _

[PATCH] D62156: [Sema][PR41730] Diagnose addr space mismatch while constructing objects

rjmccall added inline comments. Comment at: include/clang/Sema/Overload.h:977 + +void setDestAS(LangAS AS) { DestAS = AS; } }; Can this assert that `Kind == CSK_InitByConstructor || Kind == CSK_InitByUserDefinedConversion`? Comment at:

[PATCH] D62156: [Sema][PR41730] Diagnose addr space mismatch while constructing objects

rjmccall added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3653 +def note_ovl_candidate_illegal_constructor_adrspace_mismatch : Note< +"candidate constructor ignored: address space mismatch between object and constructor">; def note_ovl_candidate

[PATCH] D60748: Fix i386 struct and union parameter alignment

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Yes, LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60748/new/ https://reviews.llvm.org/D60748 ___ cfe-commits mailing list cfe

[PATCH] D61974: [ObjC] Fix encoding of ObjC pointer types that are pointers to typedefs

rjmccall added inline comments. Comment at: lib/AST/ASTContext.cpp:6975 -isa(PointeeTy.getTypePtr()) && -!Options.EncodePointerToObjCTypedef()) { - // Another historical/compatibility reason. Is this option dead now? Repository: rC Clang

[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors

rjmccall added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); Should this code be conditional to OpenCL? An

[PATCH] D61974: [ObjC] Fix encoding of ObjC pointer types that are pointers to typedefs

rjmccall added a comment. LGTM. David should sign off, too, though. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61974/new/ https://reviews.llvm.org/D61974 ___ cfe-commits mailing list cfe-commits@lists.llvm.org ht

[PATCH] D55413: [ExprConstant] Handle compound assignment when LHS has integral type and RHS has floating point type

rjmccall added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:3441 + Value); +} APSInt LHS = HandleIntToIntCast(Info, E, PromotedLHSType, Can we more fundamentally restructure this entire handler so that, if

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

rjmccall added a comment. Nice catch. This also fixes problems where there might be cleanups entered by `EmitParmDecl`, e.g. in ObjC++ with a parameter of type `struct A { __strong id x; }`; could you please add a test for that and verify that the parameter is destroyed at an appropriate point

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

rjmccall added a comment. In D54862#1328290 , @mikael wrote: > Seems like my this commit broke: > http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-buildserver/builds/33176 > > But since I don't really know what anything about lldb, I probably wo

[PATCH] D55413: [ExprConstant] Handle compound assignment when LHS has integral type and RHS has floating point type

rjmccall added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:3441 + Value); +} APSInt LHS = HandleIntToIntCast(Info, E, PromotedLHSType, cpplearner wrote: > rjmccall wrote: > > Can we more fundamentally rest

[PATCH] D54489: Implement -frecord-command-line (-frecord-gcc-switches)

rjmccall added a comment. So we're using the same command line option as GCC to produce something in the same section as GCC but formatting that section incompatibly with GCC? That combination of choices does not seem like a good idea. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D544

[PATCH] D55656: [OpenCL] Address space for default class members

rjmccall added inline comments. Comment at: lib/AST/ASTContext.cpp:2781 + + return getAddrSpaceQualType(NewT, Orig.getAddressSpace()); } You're trying to handle a method qualifier, not a type a functions that are themselves in some non-standard address space,

[PATCH] D55662: [Sema][ObjC] Do not warn about repeated uses of weak variables when the variables are accessed in an unevaluated context.

rjmccall added a comment. I think the right fix here is to call `CheckPlaceholderExpr` in `ActOnDecltypeExpression` / `HandleExprEvaluationContextForTypeof` before the original unevaluated context is exited. You can then change the type-building routines to assert that they aren't given a plac

[PATCH] D54489: Implement -frecord-command-line (-frecord-gcc-switches)

rjmccall added a comment. In D54489#1330331 , @scott.linder wrote: > In D54489#1330255 , @rjmccall wrote: > > > So we're using the same command line option as GCC to produce something in > > the same section as GC

[PATCH] D55413: [ExprConstant] Handle compound assignment when LHS has integral type and RHS has floating point type

rjmccall added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:3441 + Value); +} APSInt LHS = HandleIntToIntCast(Info, E, PromotedLHSType, cpplearner wrote: > rjmccall wrote: > > cpplearner wrote: > > > rjmcca

[PATCH] D55672: Mangle calling conventions into function pointer types where GCC does

rjmccall added a comment. Thanks. I think you missed these, though: - a template argument that's just a function type, not a function pointer or reference type - a specialization of a function template that happens to be declared with this calling convention (this also probably shouldn't inclu

[PATCH] D55672: Mangle calling conventions into function pointer types where GCC does

rjmccall added a comment. In D55672#1330458 , @rnk wrote: > In D55672#1330402 , @rnk wrote: > > > - a specialization of a function template that happens to be declared with > > this calling convention (this also pr

[PATCH] D55672: Mangle calling conventions into function pointer types where GCC does

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, that looks great. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55672/new/ https://reviews.llvm.org/D55672 ___ cfe-commits m

[PATCH] D55413: [ExprConstant] Handle compound assignment when LHS has integral type and RHS has floating point type

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, that looks good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55413/new/ https://reviews.llvm.org/D55413 ___ cfe-comm

[PATCH] D55662: [Sema][ObjC] Do not warn about repeated uses of weak variables when the variables are accessed in an unevaluated context.

rjmccall added a comment. Do you have an example, and is there a reasonable place to call `CheckPlaceholderExpr` on those paths? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55662/new/ https://reviews.llvm.org/D55662 _

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added a comment. In D53738#1333276 , @leonardchan wrote: > In D53738#1326071 , @rjmccall wrote: > > > I'm fine with making this change under the assumption that we've gotten the > > language rule right. E

[PATCH] D55656: [OpenCL] Address space for default class members

rjmccall added inline comments. Comment at: lib/AST/ASTContext.cpp:2781 + + return getAddrSpaceQualType(NewT, Orig.getAddressSpace()); } Anastasia wrote: > rjmccall wrote: > > You're trying to handle a method qualifier, not a type a functions that are > > them

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:77 + if (MD) +RecTy = Context.getAddrSpaceQualType(RecTy, MD->getType().getAddressSpace()); return Context.getPointerType(CanQualType::CreateUnsafe(RecTy)); ebevhan wrote: > I'm a bit lat

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

rjmccall added a comment. In D54862#1333700 , @ebevhan wrote: > I'm also a bit confused about the semantics that this patch is applying to > function types. It mostly seems to concern the extra trailing Qualifiers on > CXXMethodDecl to store the addrspac

[PATCH] D55781: Make CC mangling conditional on the ABI version

rjmccall added a reviewer: probinson. rjmccall added a comment. I think the core question is whether there are any vendors who care about ABI stability for these attributes who don't care about the bugfix. Speaking for Apple, we would rather have the bugfix. Sony might have different thoughts;

[PATCH] D55771: [AST] Store the callee and argument expressions of CallExpr in a trailing array.

rjmccall added a comment. I agree with not packing the argument count in. Comment at: include/clang/AST/Stmt.h:439 +/// trailing objects belonging to CallExpr. +unsigned OffsetToTrailingObjects : 6; }; If we're not packing anything into these bits an

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

rjmccall added inline comments. Comment at: clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm:10 + // Use variadic args to force inlining the inherited constructor. + Base(const Strong &s, ...) {} +}; To test what I'd like to see, this needs to take `S

[PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. You're making intrinsics with `weak_external` linkage? I feel like that's going to be unnecessarily awkward in the future, but okay. Repository: rC Clang CHANGES SINCE LAST ACTION h

[PATCH] D55771: [AST] Store the callee and argument expressions of CallExpr in a trailing array.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, looks great. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55771/new/ https://reviews.llvm.org/D55771 __

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

rjmccall added inline comments. Comment at: clang/test/CodeGenObjCXX/inheriting-constructor-cleanup.mm:25 +// CHECK-LABEL: define void @_Z1fv +// CHECK: call void (%struct.Base*, i8*, ...) @_ZN4BaseC2E6Strongz(%struct.Base* {{.*}}, i8* {{.*}}) +// CHECK-NEXT: call void @_ZN9Inhe

[PATCH] D55662: [Sema][ObjC] Do not warn about repeated uses of weak variables when the variables are accessed in an unevaluated context.

rjmccall added a comment. In D55662#1335146 , @ahatanak wrote: > Here are a couple of examples I found running the regression tests: > > int f0(int); > float f0(float); > decltype(f0) f0_a; // this is not valid. > > > > > template > int var_expr

[PATCH] D55662: [Sema][ObjC] Do not warn about repeated uses of weak variables when the variables are accessed in an unevaluated context.

rjmccall added a comment. In D55662#1335402 , @ahatanak wrote: > Sorry, please ignore my previous comment. I was looking at the wrong place. > > The following code reaches `Sema::BuildDecltypeType` without going through > `ActOnDecltypeExpression`: > >

[PATCH] D55869: Convert some ObjC retain/release msgSends to runtime calls

rjmccall added a comment. So, once upon a time this was a problem because we were rewriting the method calls in the runtime itself. Can you explain how that's not a problem now? Do we just expect the runtime to be compiled with the right switch? Comment at: include/clang/Ba

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

rjmccall added a comment. You're gating on OpenCL C++ in several places in this patch, but I don't think you need to. This extension should be available to anyone using address spaces and C++. Comment at: lib/Parse/ParseDecl.cpp:6162 +} + } + Th

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

rjmccall added inline comments. Comment at: clang/include/clang/Basic/FixedPoint.h:98 public: + APFixedPoint() = default; APFixedPoint(const llvm::APInt &Val, const FixedPointSemantics &Sema) This should be documented to describe what it does. Does it cr

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

rjmccall added inline comments. Comment at: lib/Parse/ParseDecl.cpp:6162 +} + } + Anastasia wrote: > rjmccall wrote: > > This is enforcing a restriction that users write `const __private`, which > > seems unreasonable. It looks like `ParseTypeQuali

[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

rjmccall added a comment. Thanks, LGTM. Somewhat surprised that we don't have an implicit copy of the `Strong` argument, but it's not a bad thing that we don't. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55543/new/ https://reviews.llvm.org/D55543

[PATCH] D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC

rjmccall added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3492 +def warn_ignored_objc_externally_retained : Warning< + "'objc_externally_retained' can only be applied to strong retainable " + "object pointer types with automatic storage">, InG

[PATCH] D55662: [Sema][ObjC] Do not warn about repeated uses of weak variables when the variables are accessed in an unevaluated context.

rjmccall added a comment. In D55662#1336835 , @ahatanak wrote: > In D55662#1335773 , @rjmccall wrote: > > > Okay. You may need to push an unevaluated context when doing that. > > > Since I'm just moving the call to

[PATCH] D55844: [Fixed Point Arithmetic] Fixed Point Subtraction

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3397 Value *Result; - if (ResultFixedSema.isSaturated()) { -llvm::Intrinsic::ID IID = ResultFixedSema.isSigned() - ? llvm::Intrinsic::sadd_sat -

[PATCH] D55869: Convert some ObjC retain/release msgSends to runtime calls

rjmccall added reviewers: theraven, js. rjmccall added a comment. Okay. That's also presumably true for open-source runtimes that support ARC; tagging David Chisnall and Jonathan Schleifer to get their input. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55869/new/ https://reviews.llv

[PATCH] D55869: Convert some ObjC retain/release msgSends to runtime calls

rjmccall added a comment. In D55869#1337706 , @js wrote: > Thanks for tagging me! > > The ObjFW runtime itself does not contain anything about release, retain or > autorelease - these are all part of ObjFW (as in the framework). As ObjFW > also supports

[PATCH] D55869: Convert some ObjC retain/release msgSends to runtime calls

rjmccall added a comment. In D55869#1337733 , @pete wrote: > In D55869#1337723 , @js wrote: > > > In D55869#1337711 , @dexonsmith > > wrote: > > > > > In D55869#1337706

[PATCH] D55948: Modify DeclaratorChuck::getFunction to use DeclSpec for qualifiers

rjmccall added inline comments. Comment at: include/clang/Sema/DeclSpec.h:1304 +/// DeclSpec for the function with the qualifier related info. +DeclSpec *TypeDeclSpec; + Can we give this a better name? I know we use "type qualifiers" for this concept i

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

rjmccall added inline comments. Comment at: lib/Parse/ParseDecl.cpp:6162 +} + } + Anastasia wrote: > rjmccall wrote: > > Anastasia wrote: > > > rjmccall wrote: > > > > This is enforcing a restriction that users write `const __private`, > > > > which

[PATCH] D55869: Convert some ObjC retain/release msgSends to runtime calls

rjmccall added a comment. In D55869#1338003 , @theraven wrote: > This should be fine for the GNUstep runtime (the GCC runtime doesn't support > ARC at all). My main concern is that it will break already-released versions > of the runtime built with a ne

[PATCH] D55662: [Sema][ObjC] Do not warn about repeated uses of weak variables when the variables are accessed in an unevaluated context.

rjmccall added a comment. In D55662#1338766 , @ahatanak wrote: > In D55662#1337141 , @rjmccall wrote: > > > In D55662#1336835 , @ahatanak > > wrote: > > > > > In D55662#1335

[PATCH] D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC

rjmccall added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3783 +parameters or local variables in ARC mode. When applied to parameters, it causes +clang to omit the implicit calls to objc_retain and objc_release on function +entry and exit respectivly. Whe

<    6   7   8   9   10   11   12   13   14   15   >