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

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Sure, that's fine. https://reviews.llvm.org/D49952 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mail

[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer

rjmccall added a comment. I would expect this to replace the existing warning, not to appear together with it. Repository: rC Clang https://reviews.llvm.org/D50278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi

[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer

rjmccall added inline comments. Comment at: test/Sema/conditional-expr.c:78 + // expected-error@-1{{converting '__attribute__((address_space(2))) int *' to type 'void *' changes address space of pointer}} + // expected-error@-2{{conve

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

rjmccall added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:1630 +if (const CXXDestructorDecl *DD = RD->getDestructor()) + if (const auto FPT = DD->getType()->getAs()) +// Conservatively assume the destructor can throw if the exception I

[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer

rjmccall added inline comments. Comment at: test/Sema/conditional-expr.c:78 + // expected-error@-1{{converting '__attribute__((address_space(2))) int *' to type 'void *' changes address space of pointer}} + // expected-error@-2{{conve

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

rjmccall added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:1643 +if (Ctx.getBlockVarCopyInits(VD)) + return true; + return false; ahatanak wrote: > ahatanak wrote: > > rjmccall wrote: > > > Can you just ask Sema to check `canThrow` for the exp

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

rjmccall added a comment. You might want to ask Richard on IRC if there are caveats when using that for these purposes. Repository: rC Clang https://reviews.llvm.org/D50152 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llv

[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:6522 +bool HasDifferingLAddrSpace = LAddrSpace != ResultAddrSpace; +bool HasDifferingRAddrSpace = RAddrSpace != ResultAddrSpace; + I was going to tell you to use the predicate `Qualifiers:

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

rjmccall added a comment. That is a change that Richard should definitely sign off on. Also, I'm not sure this works — is it really okay to skip the work done by `ResolveExceptionSpec` in IRGen? What does that mean, that we're just somewhat more conservative than we would otherwise be? And w

[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer

rjmccall added a comment. In https://reviews.llvm.org/D50278#1190544, @ebevhan wrote: > I think the solution to a lot of diagnostic and behavior issues with address > spaces is to remove predication on OpenCL. It's a bit odd to have a language > feature that is enabled out of the box regardless

[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: lib/Sema/SemaExpr.cpp:6522 +bool HasDifferingLAddrSpace = LAddrSpace != ResultAddrSpace; +bool HasDifferingRAddrSpace = RAddrSpace != ResultAddr

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

rjmccall added a comment. In https://reviews.llvm.org/D50152#1191777, @ahatanak wrote: > Since BlockVarCopyInits is a map with key `VarDecl *`, I think we want to add > a flag to VarDecl (NonParmVarDeclBits) that indicates whether the copy > expression can throw or not. Or is there a reason to

[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.

rjmccall added inline comments. Comment at: lib/AST/MicrosoftMangle.cpp:448 mangleVariableEncoding(VD); - else + else if (!isa(D)) llvm_unreachable("Tried to mangle unexpected NamedDecl!"); I don't think we want `ObjCInterfaceDecl`s to enter this func

[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.

rjmccall added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:1276 +InitVar->setSection(".CRT$XCLa"); +CGM.addUsedGlobal(InitVar); + } DHowett-MSFT wrote: > rjmccall wrote: > > Is the priority system not good enough? > My reading of the LLVM langu

[PATCH] D50436: Correctly initialise global blocks on Windows.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Alright, LGTM, at least until we have that backend support you mentioned. Repository: rC Clang https://reviews.llvm.org/D50436 ___ cfe-com

[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.

rjmccall added inline comments. Comment at: lib/CodeGen/CGObjCGNU.cpp:3542 + allSelectors.push_back(entry.first); +std::sort(allSelectors.begin(), allSelectors.end()); mgrang wrote: > Please use llvm::sort instead of std::sort. See > https://llvm.org/

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

rjmccall added inline comments. Comment at: include/clang/AST/ASTContext.h:248 + /// Mapping from __block VarDecls to their copy initialization expr. The + /// boolean flag indicates whether the expression can throw. + typedef llvm::DenseMaphttps://reviews.llvm.org/D50152 _

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

rjmccall added inline comments. Comment at: include/clang/AST/ASTContext.h:248 + /// Mapping from __block VarDecls to their copy initialization expr. The + /// boolean flag indicates whether the expression can throw. + typedef llvm::DenseMap Maybe you should just make a type f

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

rjmccall added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:1682 + if (IsCopyHelper && Ctx.getBlockVarCopyInits(Var).CanThrow) +Name += "c"; +} ahatanak wrote: > rjmccall wrote: > > I don't think you need to add `d` to the na

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

rjmccall added a comment. Herald added a subscriber: jfb. Hey, still working on this? Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-

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

rjmccall added inline comments. Comment at: include/clang/AST/ASTContext.h:161 +Expr *getCopyExpr() const; +bool canThrow() const; +llvm::PointerIntPair ExprAndFlag; These should all just be defined inline. Comment at: lib/CodeGen/C

[PATCH] D50527: [Parser] Support alternative operator token keyword args in Objective-C++

rjmccall added a comment. Assuming you've done enough source-compatibility testing to say with reasonable confidence that this won't break anything, I think this is fine. It's a core goal of Objective-C/C++ to allow the base language as a complete subset if at all possible. https://reviews.l

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

rjmccall added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:1725 BlockFieldFlags Flags, bool EHOnly, + bool DisposeCannotThrow, VarDecl *Var, CodeGenFunction &CGF) { ---

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

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/D50152 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.

rjmccall accepted this revision. rjmccall added a comment. LGTM. Comment at: lib/CodeGen/CGObjCGNU.cpp:915 +return name; + } /// The GCC ABI superclass message lookup function. Takes a pointer to a theraven wrote: > rjmccall wrote: > > theraven wrote:

[PATCH] D50559: [gnu-objc] Make selector order deterministic.

rjmccall added a comment. You can't test that there's no non-determinism, but you can certainly test that we emit selectors in sorted order as opposed to the order in which they're used. I imagine a function with a bunch of `@selector` expressions should be good enough for that. Repository:

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1016 + if (DstScale > SrcScale) { +// Need to allocate space before shifting left +ResultWidth = SrcWidth + DstScale - SrcScale; In IR, this isn't really "allocating" space. =

[PATCH] D50559: [gnu-objc] Make selector order deterministic.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks. I appreciate the fact that you spelled it all out in the test, too. LGTM. Comment at: lib/CodeGen/CGObjCGNU.cpp:3547 + allSelectors.push_back(entry.first);

[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields

rjmccall added a comment. We should absolutely have static assertions to check that these bit-field types don't get larger than 32 bits. A lot of the subclass layouts have been tweaked to fit that (packing into the tail padding of `Type` on 64-bit targets), so accidentally overflowing to use m

[PATCH] D50630: [AST] Update/correct the static_asserts for the bit-fields in Type

rjmccall added a comment. Oh, I missed that there was a separate review for this. A lot of the important subclasses that need extra storage have been designed with the expectation that these bit-fields fit within 32 bits. For example, `FunctionType` starts with a bunch of bit-fields because t

[PATCH] D21767: Fix instantiation of friend function templates

rjmccall added a comment. Shouldn't there just be a link in the AST from the instantiated `FunctionTemplateDecl ` back to the original pattern? Maybe a generalization of `InstantiatedFromMember` in `RedeclarablableTemplateDecl`? Repository: rC Clang https://reviews.llvm.org/D21767 _

[PATCH] D50630: [AST] Update/correct the static_asserts for the bit-fields in Type

rjmccall added a comment. In https://reviews.llvm.org/D50630#1197795, @riccibruno wrote: > @rjmccall > > I would argue that we should make these bit-fields take 8 bytes for the > following reasons: > > 1. On 64 bits archs, this is free since we have already a little less than 8 > bytes of paddi

[PATCH] D50630: [AST] Update/correct the static_asserts for the bit-fields in Type

rjmccall added a comment. In https://reviews.llvm.org/D50630#1198053, @riccibruno wrote: > I actually did exactly this. My approach was to extend what is already done, > that is add nested classes SomethingBitfields in Type and add an instance of > each to the anonymous union. The classes which

[PATCH] D50630: [AST] Update/correct the static_asserts for the bit-fields in Type

rjmccall added a comment. Sure, that seems like a reasonable optimization. Repository: rC Clang https://reviews.llvm.org/D50630 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50715: [AST] Store the OwnedTagDecl as a trailing object in ElaboratedType.

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/D50715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D21767: Fix instantiation of friend function templates

rjmccall added a comment. Oh, I see. The code currently tries to work with just the specialization and the pattern. To do the instantiation, we have to find template arguments for the context in which the pattern appears. For function temploids that aren't defined in a friend declaration, we

[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields

rjmccall added a comment. Our experience is that we keep adding more complexity to `FunctionType`, so it'd be nice if the bits weren't pressed up against the absolute limit. Dynamic exception specifications are really common, but only in the zero-exceptions case, so as long as we can efficient

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1042 +std::min(DstScale + DstFPSema.getIntegralBits(), ResultWidth)); +Value *Zero = ConstantInt::get(CGF.getLLVMContext(), APInt(ResultWidth, 0)); + You can just pass 0 here an

[PATCH] D50783: [CodeGen] Merge identical block descriptor global variables

rjmccall added a comment. In https://reviews.llvm.org/D50783#1200868, @ahatanak wrote: > A few points I forgot to mention: > > - This optimization kicks in only in NonGC mode. I don't think we need to > care much about GC anymore, so I think that's OK. Yes, that's fine. > - There is a lot of

[PATCH] D50783: [CodeGen] Merge identical block descriptor global variables

rjmccall added a comment. Okay. Repository: rC Clang https://reviews.llvm.org/D50783 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression

rjmccall added inline comments. Comment at: test/CodeGenObjC/forward-declare-protocol-gnu.m:6 -Protocol *getProtocol(void) -{ - return @protocol(P); -} +@interface I +@end arphaman wrote: > rjmccall wrote: > > Does this really not require a defin

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

rjmccall added a comment. In https://reviews.llvm.org/D50616#1203446, @leonardchan wrote: > In https://reviews.llvm.org/D50616#1202034, @ebevhan wrote: > > > I think I've mentioned this before, but I would like to discuss the > > possibility of adding a target hook(s) for some of these operation

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

rjmccall added a comment. In https://reviews.llvm.org/D50616#1203692, @ebevhan wrote: > In https://reviews.llvm.org/D50616#1203446, @leonardchan wrote: > > > Sorry I forgot to address this also. Just to make sure I understand this > > correctly since I haven't used these before: target hooks are

[PATCH] D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression

rjmccall added inline comments. Comment at: lib/CodeGen/CGObjCMac.cpp:6788 + "emitting protocol metadata without definition"); + PD = PD->getDefinition(); What happens in the `@implementation` case (the one that we're not diagnosing yet) when the prot

[PATCH] D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: lib/CodeGen/CGObjCMac.cpp:6788 + "emitting protocol metadata without definition"); + PD = PD->getDefinition(); arphaman wrot

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

rjmccall added a comment. >> Has anyone actually asked LLVM whether they would accept fixed-point types >> into IR? I'm just a frontend guy, but it seems to me that there are >> advantages to directly representing these operations in a portable way even >> if there are no in-tree targets provi

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

rjmccall added a comment. In https://reviews.llvm.org/D50616#1203772, @lebedev.ri wrote: > In https://reviews.llvm.org/D50616#1203751, @rjmccall wrote: > > > In https://reviews.llvm.org/D50616#1203692, @ebevhan wrote: > > > > > > > > > > > Has anyone actually asked LLVM whether they would accept

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

rjmccall added a comment. In https://reviews.llvm.org/D50616#1206181, @leonardchan wrote: > I made a post on llvm-dev > (http://lists.llvm.org/pipermail/llvm-dev/2018-August/125433.html) to see if > other people have opinions on this. In the meantime, do you think I should > make a separate pa

[PATCH] D50527: [Parser] Support alternative operator token keyword args in Objective-C++

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D50527#1206460, @erik.pilkington wrote: > Ping! If the build came back clean, then I think our combination of previous sign-offs is good enough. :) https://

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

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/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D51025: [CodeGen] Fix handling of variables captured by a block that is nested inside a lambda

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/D51025 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

rjmccall added a comment. In https://reviews.llvm.org/D44539#1208780, @QF5690 wrote: > In https://reviews.llvm.org/D44539#1207982, @rjmccall wrote: > > > LGTM. > > > Thanks! What I should do next? Haven't found any info in docs about it :) https://llvm.org/docs/Contributing.html It's up to you

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

rjmccall added a comment. Please read the developer policy: https://llvm.org/docs/DeveloperPolicy.html The information is on that page. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D46475: [HIP] Set proper triple and offload kind for the toolchain

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

[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang

rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:1998 +``__ATOMIC_CONSUME``, ``__ATOMIC_ACQUIRE``, ``__ATOMIC_RELEASE``, +``__ATOMIC_ACQ_REL``, or ``__ATOMIC_SEQ_CST`` following C++11 memory model semantics. + Thank you for adding this doc

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

rjmccall added a comment. I agree that the format-specifier checker is not intended to be a portability checker. Any attempt to check portability problems has to account for two things: - Not all code is intended to be portable. If you're going to warn about portability problems, you need som

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

rjmccall added a comment. In https://reviews.llvm.org/D42933#1092077, @smeenai wrote: > Apple's current recommendation for using printf with the NSInteger types is > casting to a long, per > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Strings/Articles/formatSpecif

[PATCH] D46613: _Atomic of empty struct shouldn't assert

rjmccall added inline comments. Comment at: lib/AST/ASTContext.cpp:1965 + Width = Target->getCharWidth(); + Align = Target->getCharWidth(); +} else if (Width <= Target->getMaxAtomicPromoteWidth()) { Alignment, unlike size, is definitely never 0. I

[PATCH] D46613: _Atomic of empty struct shouldn't assert

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/D46613 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D45900: CodeGen: Fix invalid bitcast for lifetime.start/end

rjmccall added a comment. In https://reviews.llvm.org/D45900#1093154, @yaxunl wrote: > In https://reviews.llvm.org/D45900#1083377, @rjmccall wrote: > > > Oh, I see, it's not that the lifetime intrinsics don't handle pointers in > > the alloca address space, it's that we might have already promot

[PATCH] D46643: CodeGen: Emit string literal in constant address space

rjmccall added a comment. The part about string literals looks fine, but: Comment at: lib/CodeGen/CGDecl.cpp:1375 +Loc = Address(EmitCastToVoidPtrInAllocaAddrSpace(Loc.getPointer()), + Loc.getAlignment()); I don't understand why a patch a

[PATCH] D46651: [OpenCL] Support placement new/delete in Sema

rjmccall added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:2030 + } +} I think a better interpretation of this rule would be to just error on attempts to use the standard non-placement operator new/delete instead of trying to outlaw the ope

[PATCH] D46241: [CodeGen] Recognize more cases of zero initialization

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprConstant.cpp:1413 + } else if (!Init->isEvaluatable(CE.CGM.getContext())) { +return false; + } else if (InitTy->hasPointerRepresentation()) { sepavloff wrote: > rsmith wrote: > > sepavloff wrote:

[PATCH] D46665: [Itanium] Emit type info names with external linkage.

rjmccall added inline comments. Comment at: lib/CodeGen/ItaniumCXXABI.cpp:3098 +InfoLinkage = getTypeInfoLinkage(CGM, Ty); +NameLinkage = getTypeInfoLinkage(CGM, Ty, /*ForName*/ true); + } I think we could probably just have the function return both inst

[PATCH] D46665: [Itanium] Emit type info names with external linkage.

rjmccall added inline comments. Comment at: lib/CodeGen/ItaniumCXXABI.cpp:3098 +InfoLinkage = getTypeInfoLinkage(CGM, Ty); +NameLinkage = getTypeInfoLinkage(CGM, Ty, /*ForName*/ true); + } rjmccall wrote: > I think we could probably just have the functio

[PATCH] D46665: [Itanium] Emit type info names with external linkage.

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

[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang

rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:1998 +``__ATOMIC_CONSUME``, ``__ATOMIC_ACQUIRE``, ``__ATOMIC_RELEASE``, +``__ATOMIC_ACQ_REL``, or ``__ATOMIC_SEQ_CST`` following C++11 memory model semantics. + rjmccall wrote: > Thank you f

[PATCH] D46241: [CodeGen] Recognize more cases of zero initialization

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/D46241 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

[PATCH] D46643: CodeGen: Emit string literal in constant address space

rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1375 +Loc = Address(EmitCastToVoidPtrInAllocaAddrSpace(Loc.getPointer()), + Loc.getAlignment()); yaxunl wrote: > rjmccall wrote: > > I don't understand why a patch about str

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

rjmccall added a comment. Can we suppress this optimization only when we can't emit an alias? An alias shouldn't degrade debugging experience, and it's good to emit less code at -O0. Repository: rC Clang https://reviews.llvm.org/D46685 ___ cfe-

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

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

[PATCH] D46740: [Clang Tablegen][RFC] Allow Early Textual Substitutions in `Diagnostic` messages.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. This seems like a good idea to me. I wonder if it would be better to take advantage of this to shrink the string tables by preserving the substitution structure until runtime, but that re

[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, that works for me. The actual semantic parts of the diff seem to have disappeared from the patch posted to Phabricator, for what it's worth. Repository: rC Clang https://revie

[PATCH] D46643: CodeGen: Emit string literal in constant address space

rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1373 + llvm::Type *BP = llvm::Type::getInt8Ty(CGM.getLLVMContext()) + ->getPointerTo(Loc.getAddressSpace()); if (Loc.getType() != BP) `CGM.Int8Ty` exists. =

[PATCH] D46643: CodeGen: Emit string literal in constant address space

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

[PATCH] D45900: CodeGen: Fix invalid bitcast for lifetime.start/end

rjmccall added a comment. That would work. I think it would be reasonable for AutoVarEmission to store that pointer if it makes things easier. https://reviews.llvm.org/D45900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llv

[PATCH] D46489: [HIP] Let assembler output bitcode for amdgcn

rjmccall added a comment. In https://reviews.llvm.org/D46489#1099979, @yaxunl wrote: > In https://reviews.llvm.org/D46489#1088940, @rjmccall wrote: > > > I think the right solution here is to make a CompileJobAction with type > > TY_LLVM_BC in the first place. You should get the advice of a dri

[PATCH] D46665: [Itanium] Emit type info names with external linkage.

rjmccall added a comment. I believe static and dynamic linkers — at least on ELF and Mach-O — will always drop weak symbols for strong ones. Now, I think that isn't LLVM's posted semantics for linkonce_odr, but to me that means that LLVM's semantics are inadequate, not that we should decline t

[PATCH] D46665: [Itanium] Emit type info names with external linkage.

rjmccall added a comment. In https://reviews.llvm.org/D46665#1102348, @rsmith wrote: > In https://reviews.llvm.org/D46665#1102290, @rjmccall wrote: > > > I believe static and dynamic linkers — at least on ELF and Mach-O — will > > always drop weak symbols for strong ones. Now, I think that isn'

[PATCH] D45900: CodeGen: Fix invalid bitcast for lifetime.start/end

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

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rjmccall added a comment. I agree that the new-expression case doesn't use the destructor, and all the other cases of list-initialization presumably use the destructor for the initialized type for separate reasons. Ok. Comment at: test/CodeGenObjCXX/arc-list-init-destruct.mm

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

rjmccall added a comment. Well, internal and external types are important cases. I'm fine with this. It's a pity that we can't express what we want with aliases, though. Repository: rC Clang https://reviews.llvm.org/D46685 ___ cfe-commits mail

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

rjmccall added a comment. It's waiting on a discussion that's happening pretty slowly, sorry. I know this is frustrating. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

rjmccall added a comment. This isn't really an Objective-C user forum, but I'll try to summarize briefly. `unsafe_unretained` is an unsafe version of `weak` — it's unsafe because it can be left dangling if the object is deallocated. It's necessary for a small (and getting smaller every year)

[PATCH] D47092: downgrade strong type info names to weak_odr linkage

rjmccall added a comment. Incomplete classes are a curse. I don't suppose we can just modify the language specification to make it illegal to use `typeid(Incomplete*)`? Or make equality/hashing undefined in these cases? Comment at: test/CodeGenCXX/arm64.cpp:48 void A::fo

[PATCH] D46740: [Clang Tablegen][RFC] Allow Early Textual Substitutions in `Diagnostic` messages.

rjmccall added inline comments. Comment at: utils/TableGen/ClangDiagnosticsEmitter.cpp:514 + std::vector Diags = Records.getAllDerivedDefinitions("Diagnostic"); + llvm::for_each(Diags, [&](Record *R) { substituteDiagText(R, SubsMap); }); EricWF wrote: > rjmcc

[PATCH] D47092: downgrade strong type info names to weak_odr linkage

rjmccall added a comment. In https://reviews.llvm.org/D47092#1105317, @rjmccall wrote: > Incomplete classes are a curse. I don't suppose we can just modify the > language specification to make it illegal to use `typeid(Incomplete*)`? Or > make equality/hashing undefined in these cases? Hone

[PATCH] D47096: CodeGen: block capture shouldn't ICE

rjmccall added a comment. RecursiveASTVisitor instantiations are huge. Can you just make the function take a Stmt and then do the first few checks if it happens to be an Expr? Repository: rC Clang https://reviews.llvm.org/D47096 ___ cfe-commits

[PATCH] D47092: downgrade strong type info names to weak_odr linkage

rjmccall added inline comments. Comment at: test/CodeGenCXX/arm64.cpp:48 void A::foo() {} - // CHECK-GLOBALS-DAG: @_ZTSN5test21AE = constant [11 x i8] + // CHECK-GLOBALS-DAG: @_ZTSN5test21AE = weak_odr constant [11 x i8] // CHECK-GLOBALS-DAG: @_ZTIN5test21AE = constant {

[PATCH] D47096: CodeGen: block capture shouldn't ICE

rjmccall added a comment. In https://reviews.llvm.org/D47096#1105374, @jfb wrote: > In https://reviews.llvm.org/D47096#1105368, @rjmccall wrote: > > > RecursiveASTVisitor instantiations are huge. Can you just make the > > function take a Stmt and then do the first few checks if it happens to be

[PATCH] D47096: CodeGen: block capture shouldn't ICE

rjmccall added a comment. Test case should go in test/CodeGenCXX. Also, there already is a `blocks.cpp` there. Repository: rC Clang https://reviews.llvm.org/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cg

[PATCH] D47099: Disable casting of alloca for ActiveFlag

rjmccall added a comment. Maybe there should just be a method that makes a primitive alloca without the casting, and then you can call that in CreateTempAlloca. https://reviews.llvm.org/D47099 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D47096: CodeGen: block capture shouldn't ICE

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/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D47099: Disable casting of alloca for ActiveFlag

rjmccall added a comment. In https://reviews.llvm.org/D47099#1105574, @yaxunl wrote: > In https://reviews.llvm.org/D47099#1105493, @rjmccall wrote: > > > Maybe there should just be a method that makes a primitive alloca without > > the casting, and then you can call that in CreateTempAlloca. > >

[PATCH] D46052: GNUstep Objective-C ABI version 2

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, my comments seem to all be addressed. Repository: rC Clang https://reviews.llvm.org/D46052 ___ cfe-commits mailing list cfe-commit

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

rjmccall added a comment. This was approved by the Objective-C language group as a default-off warning. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1018 +def warn_objc_property_assign_on_object : Warning< + "'assign' attribute must not be of object type, use 'unsafe

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

rjmccall added a comment. In https://reviews.llvm.org/D44539#1106443, @aaron.ballman wrote: > In https://reviews.llvm.org/D44539#1106152, @rjmccall wrote: > > > This was approved by the Objective-C language group as a default-off > > warning. > > > We usually do not expose new default-off warnin

[PATCH] D47099: Disable casting of alloca for ActiveFlag

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:80 auto Alloca = CreateTempAlloca(Ty, Name, ArraySize); Alloca->setAlignment(Align.getQuantity()); if (AllocaAddr) Could you change this to call CreateTempAllocaWithoutCast? https://rev

[PATCH] D47099: Call CreateTempAllocaWithoutCast for ActiveFlag

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

[PATCH] D47166: use zeroinitializer for (trailing zero portion of) large array initializers more reliably

rjmccall added a comment. I like this approach a lot. Comment at: lib/CodeGen/CGExprConstant.cpp:675 + // We have mixed types. Use a packed struct. + std::vector Types; + Types.reserve(Elements.size()); Why std::vector? Repository: rC Clang https://revi

  1   2   3   4   5   6   7   8   9   10   >