[PATCH] D49589: [UBSan] Strengthen pointer checks in 'new' expressions

rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenFunction.h:380 + /// True if sanitizer checks for current pointer value are generated. + bool PointerChecksAreEmitted = false; + sepavloff wrote: > rjmccall wrote: > > I don't think this is a good w

[PATCH] D49718: [CodeGen][ObjC] Make block copy/dispose helper function exception-safe.

rjmccall added a comment. In https://reviews.llvm.org/D49718#1173883, @ahatanak wrote: > Address review comments. > > I think I should be able to merge pushBlockObjectRelease and > enterByrefCleanup, but the BlockFieldFlags that are passed are different. We > set BLOCK_FIELD_IS_WEAK in addition

[PATCH] D49718: [CodeGen][ObjC] Make block copy/dispose helper function exception-safe.

rjmccall added a comment. In https://reviews.llvm.org/D49718#1174281, @ahatanak wrote: > Merge pushBlockObjectRelease and enterByrefCleanup. > > In https://reviews.llvm.org/D49718#1174113, @rjmccall wrote: > > > Heh, okay. It looks like the runtime doesn't do anything different, but I > > think

[PATCH] D49403: More aggressively complete RecordTypes with Function Pointers

rjmccall added a comment. In https://reviews.llvm.org/D49403#1175350, @olga.chupina wrote: > I should probably add one more example to explain my point of view. > Suppose we have an indirect call in the program and we need to know all > possible goals for this indirect call. Then we would like

[PATCH] D49589: [UBSan] Strengthen pointer checks in 'new' expressions

rjmccall added inline comments. Comment at: lib/CodeGen/Address.h:46 +return IsChecked; + } + `Address` is a pretty low-level type to be changing here. Is this necessary? Can you find a way to propagate this just in higher-level types like `AggValueSlot`

[PATCH] D49718: [CodeGen][ObjC] Make block copy/dispose helper function exception-safe.

rjmccall added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:2582 /// yet; if a cleanup is required for the variable itself, that needs /// to be done externally. +void CodeGenFunction::enterByrefCleanup(CleanupKind Kind, Address Addr, Please document t

[PATCH] D49718: [CodeGen][ObjC] Make block copy/dispose helper function exception-safe.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. Repository: rC Clang https://reviews.llvm.org/D49718 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

[PATCH] D49718: [CodeGen][ObjC] Make block copy/dispose helper function exception-safe.

rjmccall added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:2582 /// yet; if a cleanup is required for the variable itself, that needs /// to be done externally. +void CodeGenFunction::enterByrefCleanup(CleanupKind Kind, Address Addr, ahatanak wrote: >

[PATCH] D49868: [Sema] Fix a crash by completing a type before using it

rjmccall added a comment. Could you include a test that satisfies the exception but requires a template to be instantiated to check that? It's probably already tested elsewhere, but still. Repository: rC Clang https://reviews.llvm.org/D49868 _

[PATCH] D49589: [UBSan] Strengthen pointer checks in 'new' expressions

rjmccall added inline comments. Comment at: lib/CodeGen/CGClass.cpp:2190 This, Args, AggValueSlot::MayOverlap, - E->getLocation()); + E->getLocation(), true); } Please include `/* */` comm

[PATCH] D49868: [Sema] Fix a crash by completing a type before using it

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. https://reviews.llvm.org/D49868 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/l

[PATCH] D49589: [UBSan] Strengthen pointer checks in 'new' expressions

rjmccall added a comment. Okay, thank you, that's better, but I should've been clearer. What I was really asking for was this: in *all* the code you're adding in this patch, where you've currently written `IsChecked` or `isChecked` or `NewPointerIsChecked`, please instead write `IsSanitizerChe

[PATCH] D49589: [UBSan] Strengthen pointer checks in 'new' expressions

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, that'll do. Repository: rC Clang https://reviews.llvm.org/D49589 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://li

[PATCH] D49952: Check for NULL Destination-Type when creating ArrayConstant

rjmccall added a comment. I think the right fix here is to ensure that `DestType` is non-null at a higher level. Older branches of the compiler seem to be able to correctly emit this, probably because the initializer generated for this field ends up having type `a[0]`. Maybe we've just done s

[PATCH] D50050: [AST] CastExpr: BasePathSize is not large enough.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: include/clang/AST/Expr.h:2791 +public: + using BasePathSizeTy = unsigned short; + static_assert(std::numeric_limits::max() >= 16384,

[PATCH] D49403: More aggressively complete RecordTypes with Function Pointers

rjmccall added a comment. LLVM is sometimes confused by bitcasts of function pointers, but that's just a weird exception to the basic rule that pointer element types aren't supposed to actually matter in LLVM IR. There's a (stalled, unfortunately) effort to remove pointer element types from IR

[PATCH] D50003: Sema: Fix explicit address space cast involving void pointers

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. One minor request; feel free to just do that when you commit. Comment at: lib/Sema/SemaCast.cpp:1050 + SrcType->getAs()->getPointeeType().getAddressSpace() != +

[PATCH] D49952: Check for NULL Destination-Type when creating ArrayConstant

rjmccall added inline comments. Comment at: test/CodeGenCXX/empty-struct-init-list.cpp:11 +} c; +c d{ }; Please make this a `FileCheck` test that tests the actual compiler output. https://reviews.llvm.org/D49952 __

[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions

rjmccall added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:1638 +switch (E.Kind) { +case BlockCaptureEntityKind::CXXRecord: { + Name += "c"; I forget whether this case has already filtered out trivial copy-constructors, but if not, you sho

[PATCH] D49403: More aggressively complete RecordTypes with Function Pointers

rjmccall added a comment. My point is that IR pointer type information can't be "incorrect", because there is no semantic meaning for pointer types in LLVM IR. That has never been a part of the semantics of LLVM IR. Even if it were possible to change that — and I don't think it is, not withou

[PATCH] D50003: Sema: Fix explicit address space cast involving void pointers

rjmccall added inline comments. Comment at: lib/Sema/SemaCast.cpp:1050 + SrcType->getAs()->getPointeeType().getAddressSpace() != + DestType->getAs()->getPointeeType().getAddressSpace(); +} yaxunl wrote: > rjmccall wrote: > > I know the code w

[PATCH] D50003: Sema: Fix explicit address space cast involving void pointers

rjmccall added inline comments. Comment at: lib/Sema/SemaCast.cpp:1050 + SrcType->getAs()->getPointeeType().getAddressSpace() != + DestType->getAs()->getPointeeType().getAddressSpace(); +} yaxunl wrote: > rjmccall wrote: > > yaxunl wrote: > >

[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions

rjmccall added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:1638 +switch (E.Kind) { +case BlockCaptureEntityKind::CXXRecord: { + Name += "c"; ahatanak wrote: > rjmccall wrote: > > I forget whether this case has already filtered out trivial

[PATCH] D49403: More aggressively complete RecordTypes with Function Pointers

rjmccall added a comment. Sorry, let me be more precise. The semantics of LLVM IR are that the element type of a pointer matters when doing specific operations on pointers: loads, stores, GEPs, calls, and so on. Many of these operations have been at least partially updated — in some combinati

[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions

rjmccall added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:1691 + +Name += "_" + llvm::to_string(E.Capture->getOffset().getQuantity()); + } I feel like this reads a little better if you write the quantity first. Also I think you can drop the unde

[PATCH] D50286: avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end calls

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:521 + CGM.getCodeGenOpts().OptimizationLevel > 0 && + !CGM.getCodeGenOpts().SanitizeAddressUseAfterScope) { +OldConditional = OutermostConditional; Why only when optimiza

[PATCH] D49952: Check for NULL Destination-Type when creating ArrayConstant

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprConstant.cpp:2123 +// This means that the array type is probably "IncompleteType" or some +// type that is not ConstantArray. +if (CAT == nullptr && CommonElementType == nullptr && !NumInitElts) { -

[PATCH] D50286: avoid creating conditional cleanup blocks that contain only @llvm.lifetime.end calls

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D50286 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D43013: ASan+operator new[]: Fix operator new[] cookie poisoning

rjmccall added a comment. I don't understand why your description of this patch mentions the void* placement new[] operator. There's no cookie to poison for that operator. Repository: rC Clang https://reviews.llvm.org/D43013 ___ cfe-commits mai

[PATCH] D42614: AST: support ObjC lifetime qualifiers in MS ABI

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D42614#1001279, @compnerd wrote: > @rjmccall, I've updated the approach and no longer abuse the existing > decoration styles. This uses a custom namespace with

[PATCH] D43013: ASan+operator new[]: Fix operator new[] cookie poisoning

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, thanks. LGTM. Repository: rC Clang https://reviews.llvm.org/D43013 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://l

[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method

rjmccall added a comment. Thanks, that looks a lot better. One minor piece of feedback, but otherwise LGTM. Comment at: lib/Sema/SemaExpr.cpp:4402 +Qualifiers MemberQuals = +Context.getCanonicalType(ResultType).getQualifiers(); +Qualifiers Combined = BaseQuals

[PATCH] D42549: [CodeGen] Use the zero initializer instead of storing an all zero representation.

rjmccall added a comment. I'm somewhat surprised LLVM doesn't already canonicalize this, but ok. Should we also do this when building a constant struct? Comment at: lib/CodeGen/CGExprConstant.cpp:873 Elts.push_back(C); + if (!C->isNullValue()) +AllNullValue

[PATCH] D42549: [CodeGen] Use the zero initializer instead of storing an all zero representation.

rjmccall added a comment. Oh. It's not a good idea to try to match exact local IR names like this for two reasons: - First, many IR names are different in builds with and without assertions. - Second, it's pretty susceptible to innocuous changes in output. You should use FileCheck patterns (`[

[PATCH] D42549: [CodeGen] Use the zero initializer instead of storing an all zero representation.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. No worries. LGTM! https://reviews.llvm.org/D42549 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mail

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. No, this LGTM. https://reviews.llvm.org/D41553 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/

[PATCH] D43181: [CodeGen] Initialize large arrays by copying from a global

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:421 + // with explicit initializers should be large enough. + if (NumInitElements > 8 && elementType->isBuiltinType()) { +CodeGen::CodeGenModule &CGM = CGF.CGM; Is there a good reason to

[PATCH] D43181: [CodeGen] Initialize large arrays by copying from a global

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:421 + // with explicit initializers should be large enough. + if (NumInitElements > 8 && elementType->isBuiltinType()) { +CodeGen::CodeGenModule &CGM = CGF.CGM; kosarev wrote: > rjmccall

[PATCH] D43187: [AST] Refine the condition for element-dependent array fillers

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rL LLVM https://reviews.llvm.org/D43187 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cg

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1102 +PDK_Struct // non-trivial C struct. + }; + This is unused. Comment at: lib/CodeGen/CGBlocks.cpp:1565 // For all other types, the memcpy is fine. return

[PATCH] D43089: clang: Add ARCTargetInfo

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:8185 +unsigned FreeRegs) const { + // TODO: C++ ABI? + unsigned SizeInRegs = (getContext().getTypeSize(Ty) + 31) / 32; Please do go ahead an

[PATCH] D34367: CodeGen: Fix address space of indirect function argument

rjmccall added a comment. That's not really okay; there are some places that make guarantees about call-argument ordering, and in general we want that to be decided at a higher level, rather than having a particular order forced in corner cases just to satisfy a lower-level implementation requi

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Decl.h:3619 +return NonTrivialToPrimitiveDestructiveMove; + } + Please document that this means a C++-style destructive move that leaves the source object in a validly-initialized state. ==

[PATCH] D43181: [CodeGen] Initialize large arrays by copying from a global

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, thanks! https://reviews.llvm.org/D43181 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/l

[PATCH] D42366: [CodeGen] Fix generation of TBAA tags for may-alias accesses

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I'm fine with that plan. LGTM. https://reviews.llvm.org/D42366 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

[PATCH] D43430: Omit nullptr check for sufficiently simple delete-expressions

rjmccall added a comment. Discourse nitpick: I would encourage you to just use the ordinary phrase "null pointer", or just "null", when referring to a pointer value that happens to be null and to reserve "nullptr" for *statically* null pointers, especially the `nullptr` expression. If the poin

[PATCH] D42366: [CodeGen] Fix generation of TBAA tags for may-alias accesses

rjmccall added a comment. In https://reviews.llvm.org/D42366#1014157, @kosarev wrote: > I think zero would serve better as the unknown-size value. People who are not > aware of TBAA internals would guess that since zero-sized accesses make no > sense, they are likely to have some special meanin

[PATCH] D34367: CodeGen: Fix address space of indirect function argument

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3505 +if (AS != LangAS::Default && AS != LangAS::opencl_private && +AS != CGM.getASTAllocaAddressSpace()) + Flag = CallArg::ByValArgNeedsCopy; This is an odd condition. What are

[PATCH] D34367: CodeGen: Fix address space of indirect function argument

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3510 + args.add(RValue::getAggregate(Dest.getAddress()), type, L); } return; Please move all this conditionality to the call-emission code; just check whether you have a load of an

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1108 +PCK_ARCStrong, // objc strong pointer. +PCK_Struct // non-trivial C struct. + }; These should all be /// documentation comments, and they mostly shouldn't talk about fields si

[PATCH] D43586: CodeGen: handle blocks correctly when inalloca'ed

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I hate `inalloca` so much. Okay. Repository: rC Clang https://reviews.llvm.org/D43586 ___ cfe-commits mailing list cfe-commits@lists.llvm

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1121 + /// after it is moved, as opposed to a truely destructive move in which the + /// source object is placed in an uninitialized state. + PrimitiveCopyKind isNonTrivialToPrimitiveDestructiveMove() const;

[PATCH] D34367: CodeGen: Fix address space of indirect function argument

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3446 + return LV.asAggregateRValue(); +} + No, I don't think this is right. This method should always return an independent RValue; if the CallArg is storing an LValue, it should copy from it

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks! This looks ready; thank you for your patience in working this out. Comment at: include/clang/AST/Type.h:1108 +PCK_ARCStrong, // objc strong pointer. +PC

[PATCH] D43430: Omit nullptr check for sufficiently simple delete-expressions

rjmccall added a comment. That is an extremely Google-specific analysis, actually; AFAIK almost nobody else uses static linking all the way down to and including the C and C++ standard libraries unless they're literally building an executable for a fully-static environment, like the kernel. Th

[PATCH] D33328: [CodeGen] Pessimize aliasing for union members (and may-alias) objects

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:1436 +if (BaseInfo.getMayAlias()) + TBAAInfo = CGM.getTBAAInfo(getContext().CharTy); llvm::MDNode *TBAAPath = CGM.getTBAAStructTagInfo(TBAABaseType, TBAAInfo, kparzysz wrote: > rjmcc

[PATCH] D33328: [CodeGen] Pessimize aliasing for union members (and may-alias) objects

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, that looks great. Repository: rL LLVM https://reviews.llvm.org/D33328 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http

[PATCH] D31830: Emit invariant.group.barrier when using union field

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3517 +CGM.getCodeGenOpts().StrictVTablePointers && +CGM.getCodeGenOpts().OptimizationLevel > 0) + addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()), Prazek

[PATCH] D33437: Emit available_externally vtables opportunistically

rjmccall added inline comments. Comment at: include/clang/AST/VTableBuilder.h:160 + "GlobalDecl can be created only from virtual function"); +if (getKind() == CK_FunctionPointer) + return GlobalDecl(getFunctionDecl()); Please use an exhaustive

[PATCH] D31830: Emit invariant.group.barrier when using union field

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Looks great, thanks. One request for additional tests, but feel free to commit when that's ready. Comment at: test/CodeGenCXX/strict-vtable-pointers.cpp:258 + take(u.h

[PATCH] D33437: Emit available_externally vtables opportunistically

rjmccall added inline comments. Comment at: include/clang/AST/VTableBuilder.h:160 + "GlobalDecl can be created only from virtual function"); +if (getKind() == CK_FunctionPointer) + return GlobalDecl(getFunctionDecl()); Prazek wrote: > rjmccall

[PATCH] D33698: [CodeGen][ObjC] Fix assertion failure in CodeGenFunction::EmitARCStoreStrongCall

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D33698 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D33706: [AMDGPU] Fix address space for global and temporary variables in C++

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. Please split this up into two patches: - one for your target-specific changes to the global address space and so on - one for the generic fix to CreateTempAlloca etc. =

[PATCH] D33437: Emit available_externally vtables opportunistically

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Looks great, thanks! https://reviews.llvm.org/D33437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/ma

[PATCH] D31830: Emit invariant.group.barrier when using union field

rjmccall added inline comments. Comment at: test/CodeGenCXX/strict-vtable-pointers.cpp:263 + +struct VirtualInVBase : virtual Empty, HoldingVirtuals { +}; "virtual" applies to an individual base, not to all following bases. You need "virtual HoldingVirtuals" he

[PATCH] D31830: Emit invariant.group.barrier when using union field

rjmccall accepted this revision. rjmccall added a comment. Thanks, LGTM. https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33735: [DebugInfo] Add ThisOrSelf attribute for emission of FlagObjectPointer.

rjmccall added a comment. In https://reviews.llvm.org/D33735#770318, @aaron.ballman wrote: > In https://reviews.llvm.org/D33735#770296, @ABataev wrote: > > > In https://reviews.llvm.org/D33735#770288, @aaron.ballman wrote: > > > > > Can you help me to understand what problem is being solved with

[PATCH] D33706: CodeGen: Cast temporary variable to proper address space

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3805 +Address Addr = +CreateMemTemp(I->Ty, ArgInfo.getIndirectAlign(), "tmp", false); IRCallArgs[FirstIRArg] = Addr.getPointer(); How about "indirect-arg-temp" as the

[PATCH] D33706: CodeGen: Cast temporary variable to proper address space

rjmccall added a comment. Thanks. One tweak and then LGTM. Comment at: lib/CodeGen/CodeGenFunction.h:1937 + /// to the stack. + + /// Because the address of a temporary is often exposed to the program in This line should have ///. https://reviews.llvm.org/

[PATCH] D33735: [DebugInfo] Add ThisOrSelf attribute for emission of FlagObjectPointer.

rjmccall added inline comments. Comment at: include/clang/AST/Decl.h:1388 SourceLocation IdLoc, IdentifierInfo *Id, - QualType T); + QualType T, bool IsThisOrSelf = false); -

[PATCH] D33706: CodeGen: Cast temporary variable to proper address space

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks! https://reviews.llvm.org/D33706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

[PATCH] D33842: [AMDGPU] Fix address space of global variable

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:360 + CGF.getContext().getTargetAddressSpace(LangAS::opencl_constant); +} auto *GV = new llvm::GlobalVariable( This is not an appropriate place to stick target-specif

[PATCH] D33735: [DebugInfo] Add ThisOrSelf attribute for emission of FlagObjectPointer.

rjmccall added inline comments. Comment at: include/clang/AST/Decl.h:1387 +IPK_CapturedContext, /// Parameter for captured context +IPK_GeneralParam,/// General implicit parameter + }; I would just call this "Other" and document it as being for kinds

[PATCH] D17215: [Sema] Fix PR14211 Crash for explicit instantiation of overloaded template function within class template

rjmccall added inline comments. Comment at: lib/Sema/SemaTemplate.cpp:8985 +// FIXME: Would this be considered an almost match as well? +continue; } These continues that you're adding are already at the end of the loop body, so this code

[PATCH] D17215: [Sema] Fix PR14211 Crash for explicit instantiation of overloaded template function within class template

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, looks great. Do you still not have commit access? https://reviews.llvm.org/D17215 ___ cfe-commits mailing list cfe-commits@lists.llv

[PATCH] D33735: [DebugInfo] Add ThisOrSelf attribute for emission of FlagObjectPointer.

rjmccall added inline comments. Comment at: include/clang/AST/Decl.h:1387 +IPK_CapturedContext, /// Parameter for captured context +IPK_GeneralParam,/// General implicit parameter + }; ABataev wrote: > rjmccall wrote: > > I would just call this "Othe

[PATCH] D17215: [Sema] Fix PR14211 Crash for explicit instantiation of overloaded template function within class template

rjmccall added a comment. Sure thing, r304951. https://reviews.llvm.org/D17215 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33735: [DebugInfo] Add ThisOrSelf attribute for emission of FlagObjectPointer.

rjmccall accepted this revision. rjmccall added inline comments. Comment at: include/clang/AST/Decl.h:901 +/// member functions. +unsigned ImplicitParamKind : 3; }; aaron.ballman wrote: > rjmccall wrote: > > ABataev wrote: > > > aaron.ballman wrote: >

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

rjmccall added a comment. Looks like you haven't introduced proper constants in the header for scopes. Comment at: docs/LanguageExtensions.rst:1855 +atomic builtins are in ``explicit`` form of the corresponding OpenCL 2.0 +builtin function, and are named with a ``__opencl__`` p

[PATCH] D17143: [Sema] PR25156 Crash when parsing dtor call on incomplete type

rjmccall added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:192 Found.clear(); -if (Step == 0 && LookupCtx) +if (Step == 0 && LookupCtx && !RequireCompleteDeclContext(SS, LookupCtx)) LookupQualifiedName(Found, LookupCtx); You should p

[PATCH] D17143: [Sema] PR25156 Crash when parsing dtor call on incomplete type

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, looks great. If you're going to be submitting multiple patches, you should really ask for commit access; it's not an arduous process. https://reviews.llvm.org/D17143 _

[PATCH] D34198: Fix __has_trivial_destructor crash when the type is incomplete with unknown array bounds.

rjmccall added a comment. Test cases? https://reviews.llvm.org/D34198 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34198: Fix __has_trivial_destructor crash when the type is incomplete with unknown array bounds.

rjmccall added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:4128 +return true; +} + I don't understand the difference you're creating between traits here. Three specific traits about destructibility allow incomplete array types regardless

[PATCH] D34367: CodeGen: Fix address space of indirect function argument

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1605 + ? CGM.getDataLayout().getAllocaAddrSpace() + : getContext().getTargetAddressSpace(LangAS::Default)); break; Everything about your reasoning seems to apply t

[PATCH] D33842: [AMDGPU] Fix address space of global variable

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:959 + /// \brief Return the target address space which is read only and can be + /// casted to the generic address space. + virtual llvm::Optional getTargetConstantAddressSpace() const { -

[PATCH] D34367: CodeGen: Fix address space of indirect function argument

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1605 + ? CGM.getDataLayout().getAllocaAddrSpace() + : getContext().getTargetAddressSpace(LangAS::Default)); break; yaxunl wrote: > rjmccall wrote: > > Everything a

[PATCH] D34198: Fix __has_trivial_destructor crash when the type is incomplete with unknown array bounds.

rjmccall added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:4128 +return true; +} + puneetha wrote: > rjmccall wrote: > > I don't understand the difference you're creating between traits here. > > Three specific traits about destructibility

[PATCH] D34454: SwiftCC: Perform physical layout when computing coercion types

rjmccall added a comment. Looks great, thanks! https://reviews.llvm.org/D34454 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33842: [AMDGPU] Fix address space of global variable

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:969 + /// LangAS::Default. + virtual unsigned getGlobalAddressSpace() const { return LangAS::Default; } + I'm not sure this really needs to exist. Comment at: includ

[PATCH] D41050: Fix over-release of return value of lambda implicitly converted to block/function pointer

rjmccall added a comment. In https://reviews.llvm.org/D41050#953917, @danzimm wrote: > Change tests to use non-O2 generated IR. It looks like the combined > objc_retainAutoreleasedReturnValue/objc_autoreleaseReturnValue calls > annihilate each other and we just get a call/ret. Is that really

[PATCH] D41050: Fix over-release of return value of lambda implicitly converted to block/function pointer

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Heh, alright, that works. It's unfortunate that -disable-llvm-passes doesn't suppress running the pass under -O0; that seems like an oversight. Anyway, LGTM. Repository: rC Clang htt

[PATCH] D41050: Fix over-release of return value of lambda implicitly converted to block/function pointer

rjmccall added a comment. Oh, yes, that could be. Repository: rC Clang https://reviews.llvm.org/D41050 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41050: Fix over-release of return value of lambda implicitly converted to block/function pointer

rjmccall closed this revision. rjmccall added a comment. Sure, r320721. Repository: rC Clang https://reviews.llvm.org/D41050 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36790: [ObjC] Messages to 'self' in class methods that return 'instancetype' should use the pointer to the class as the result type of the message

rjmccall added inline comments. Comment at: lib/Sema/SemaExprObjC.cpp:1357 +(ReceiverType->isObjCClassOrClassKindOfType() || + ReceiverType->isObjCQualifiedClassType()) && +Receiver->isObjCSelfExpr() && getLangOpts().ObjCAutoRefCount) { Wh

[PATCH] D36790: [ObjC] Messages to 'self' in class methods that return 'instancetype' should use the pointer to the class as the result type of the message

rjmccall added a comment. LGTM outside of a comment request; please feel free to commit when you'd made that change. Comment at: lib/Sema/SemaExprObjC.cpp:1361 +// pointer to the parent interface of the method when ARC is enabled ( +// because self can't be reassigned

[PATCH] D41301: ASan+operator new[]: Fix operator new[] cookie poisoning

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D41301 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D41311: [CodeGen] Fix crash when a function taking transparent union is redeclared.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2321 + !isa(ConvertType(Arg->getType())) && ArgI.getCoerceToType() == ConvertType(Ty) && ArgI.getDirectOffset() == 0) { I think the right fix is to change the

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added a comment. You should add a __has_feature check for this (__has_feature(objc_arc_fields)?), as well as documentation. The documentation would go in the ARC spec. Comment at: include/clang/AST/Decl.h:3575 + /// Functions to query basic properties of non-trivia

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added a comment. I accidentally hit 'send' too early on my review, so here's part two. I still haven't fully reviewed the new IRGen file. Comment at: lib/CodeGen/CodeGenCStruct.cpp:27 + FK_Array // array that has non-trivial elements. +}; +} There

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1421 +destroyer = CodeGenFunction::destroyNonTrivialCStruct; +cleanupKind = getARCCleanupKind(); +break; ahatanak wrote: > rjmccall wrote: > > This can only be getARCCleanupKind() if i

<    1   2   3   4   5   6   7   8   9   10   >