[PATCH] D60454: [OpenCL] Prevent mangling kernel functions

rjmccall added a comment. I think it would be more reasonable to just change `getDeclLanguageLinkage` to check for a kernel function. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60454/new/ https://reviews.llvm.org/D60454 ___ cfe-commits

[PATCH] D60302: [CodeGen][ObjC] Emit the retainRV marker as a module flag instead of named metadata.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay. In that case, I have no objections to this. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60302/new/ https://reviews.llvm.org/D60302

[PATCH] D60302: [CodeGen][ObjC] Emit the retainRV marker as a module flag instead of named metadata.

rjmccall added a comment. In D60302#1460379 , @ahatanak wrote: > Thanks. Does this require changes to swift too? Yes, it will, to `getObjCRetainAutoreleasedReturnValueMarker` in `GenObjC.cpp`. Repository: rC Clang CHANGES SINCE LAST ACTION https:/

[PATCH] D60161: Expose non-trivial struct helpers for Swift.

rjmccall added a reviewer: ahatanak. rjmccall added inline comments. Herald added a subscriber: dexonsmith. Comment at: clang/include/clang/CodeGen/CodeGenABITypes.h:140 +QualType QT); + } // end namespace CodeGen

[PATCH] D60161: Expose non-trivial struct helpers for Swift.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60161/new/ https://reviews.llvm.org/D60161 __

[PATCH] D57072: Don't codegen an unreachable return block

rjmccall commandeered this revision. rjmccall edited reviewers, added: bmoody; removed: rjmccall. rjmccall added a comment. This revision now requires review to proceed. Committed as r358104, thanks for your patience. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D59988: [PR41276] Generate address space cast of 'this' for objects attributed by an address space in C++

rjmccall added inline comments. Comment at: test/CodeGenCXX/address-space-of-this.cpp:9 +//CHECK: call void @_ZN6MyTypeC1Ei(%struct.MyType* addrspacecast (%struct.MyType addrspace(10)* @m to %struct.MyType*), i32 123) +MyType __attribute__((address_space(10))) m = 123; -

[PATCH] D58016: fixes copy constructor generation of classes containing 0-length arrays followed by exactly 1 trivial field (fixes #40682)

rjmccall closed this revision. rjmccall added a comment. Committed as r358115. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58016/new/ https://reviews.llvm.org/D58016 ___ cfe-commits mailing list cfe-

[PATCH] D60161: Expose non-trivial struct helpers for Swift.

rjmccall closed this revision. rjmccall added a comment. Committed as r358132. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60161/new/ https://reviews.llvm.org/D60161 ___ cfe-commits mailing list cfe-

[PATCH] D60542: Add support for attributes on @implementations in Objective-C

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

[PATCH] D60544: Support objc_nonlazy_class attribute on Objective-C implementations

rjmccall added a comment. Should we have a special `has_feature` test to check that this attribute is allowed on implementations? We've done that in the past when expanding the set of subjects for an attribute. But that's not necessary if we haven't made this attribute part of a public Xcode

[PATCH] D60544: Support objc_nonlazy_class attribute on Objective-C implementations

rjmccall accepted this revision. rjmccall added a comment. Great, SGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60544/new/ https://reviews.llvm.org/D60544 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D60548: Variable auto-init: also auto-init alloca

rjmccall added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:57 + case LangOptions::TrivialAutoVarInitKind::Pattern: +Byte = CGF.Builder.getInt8(0xAA); +break; Can this value be taken from some common source with the existing code? I mean, eve

[PATCH] D60548: Variable auto-init: also auto-init alloca

rjmccall added inline comments. Comment at: lib/CodeGen/PatternInit.cpp:17 + +llvm::Constant *patternFor(CodeGenModule &CGM, llvm::Type *Ty) { + // The following value is a guaranteed unmappable pointer value and has a Please use a qualified declaration here ins

[PATCH] D60548: Variable auto-init: also auto-init alloca

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: lib/CodeGen/PatternInit.h:22 + +llvm::Constant *patternFor(CodeGenModule&, llvm::Type*); + jfb wrote: > rjmccall wrote: > > Please choo

[PATCH] D60573: [Sema] ADL: Associated namespaces for class types and enumeration types (CWG 1691)

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Hmm. Does this never impact code that's just using a locally-defined type within its scope? I guess if ADL is involved, unqualified lookup must have reached the scope of the innermost na

[PATCH] D60573: [Sema] ADL: Associated namespaces for class types and enumeration types (CWG 1691)

rjmccall added a comment. In D60573#1463641 , @riccibruno wrote: > In D60573#1463569 , @rjmccall wrote: > > > Hmm. Does this never impact code that's just using a locally-defined type > > within its scope? I gues

[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:2580 +if (const BlockDecl *BD = CurContext->getInnerMostBlockDecl()) + if (!getDiagnostics().isIgnored(diag::warn_implicitly_retains_self, Loc)) +ImplicitlyRetainedSelfLocs.push_back({

[PATCH] D58236: Make address space conversions a bit stricter.

rjmccall accepted this revision. rjmccall added a comment. Alright, this seems reasonable to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58236/new/ https://reviews.llvm.org/D58236 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape

rjmccall added inline comments. Comment at: include/clang/AST/DeclBase.h:1798 + /// innermost enclosing BlockDecl or null if there are no enclosing blocks. + const BlockDecl *getInnerMostBlockDecl() const { +const DeclContext *Ctx = this; "innermost" is one

[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape

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

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

rjmccall added a subscriber: dexonsmith. rjmccall added a comment. I suspect Darwin also doesn't want to take this. We care very little about 32-bit Intel, and part of caring very little is not wanting to spend any effort dealing with the ramifications of ABI breaks. That would apply both to t

[PATCH] D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes

rjmccall added a comment. It seems reasonable to me for target hooks to run after global hooks, but can I ask why AMDGPU specifically relies on this? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60967/new/ https://reviews.llvm.org/D60967

[PATCH] D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes

rjmccall added a comment. Shouldn't it be an error if the user tries to give it hidden visibility? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60967/new/ https://reviews.llvm.org/D60967 ___ cfe-commits mailing list

[PATCH] D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes

rjmccall added a comment. In D60967#1476057 , @scott.linder wrote: > In D60967#1476029 , @rjmccall wrote: > > > Shouldn't it be an error if the user tries to give it hidden visibility? > > > We effectively consider

[PATCH] D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes

rjmccall added a comment. I suspect that other OpenCL and CUDA implementations don't care at all about symbol visibility for device-side code generation, and giving kernel functions default visibility seems like the right thing to do for the (relatively few) things at the AST level that are sen

[PATCH] D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Yeah, that's fine. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60967/new/ https://reviews.llvm.org/D60967

[PATCH] D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes

rjmccall added a comment. Yeah, that seems like a missing warning. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60967/new/ https://reviews.llvm.org/D60967 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http

[PATCH] D61147: [Sema][ObjC] Add a flavor of -Wunused-parameter that doesn't diagnose unused parameters of ObjC methods

rjmccall added a comment. Yeah, I tend to think we should just suppress this unconditionally in Objective-C. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61147/new/ https://reviews.llvm.org/D61147 ___ cfe-commits m

[PATCH] D61147: [Sema][ObjC] Add a flavor of -Wunused-parameter that doesn't diagnose unused parameters of ObjC methods

rjmccall added a comment. Sorry, yes, I meant in Objective-C methods, of course, not unconditionally even in C-like code whenever Objective-C is enabled. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61147/new/ https://reviews.llvm.org/D61147

[PATCH] D61147: [Sema][ObjC] Add a flavor of -Wunused-parameter that doesn't diagnose unused parameters of ObjC methods

rjmccall added a comment. I do not think the ObjC version of this diagnostic is useful as it's been explained here, and I would strongly recommend just not including such a warning for the time being. I would also recommend that you go fix the warning to never fire on virtual C++ methods. Re

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

rjmccall added a comment. Are you sure these are the right semantics for `nodestroy`? I think there's a reasonable argument that we should not destroy previous elements of a `nodestroy` array just because an element constructor throws. It's basically academic for true globals because the exce

[PATCH] D61147: [Sema][ObjC] Add a flavor of -Wunused-parameter that doesn't diagnose unused parameters of ObjC methods

rjmccall added a comment. In D61147#1479940 , @erik.pilkington wrote: > In D61147#1479932 , @rjmccall wrote: > > > I do not think the ObjC version of this diagnostic is useful as it's been > > explained here, and

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

rjmccall added a comment. I believe at least one of the goals of `nodestroy` is to allow the type to potentially not provide a destructor at all, so if we're going to implicitly require the destructor anyway in certain situations, we should clearly document that, and we should be aware that we

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

rjmccall added a comment. Okay, SGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61165/new/ https://reviews.llvm.org/D61165 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/ma

[PATCH] D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes

rjmccall added a comment. It seems to fine just forbid `hidden`. Again, I suspect other targets do not care because they are not using a standard dynamic loader to load the code containing kernel functions. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60967/ne

[PATCH] D61280: Variable auto-init: don't initialize aggregate padding of all aggregates

rjmccall added a comment. IIRC, C says *members* are initialized as if they were in static storage, which might mean that their interior padding should be zeroed, but I don't think says anything about the padding in the enclosing aggregate. But I think zero-initializing padding is probably the

[PATCH] D60930: [codeview] Fix symbol names for dynamic initializers and atexit stubs

rjmccall added a comment. In Swift we basically shove *everything* through our corresponding linkage-entity structure, so I'm not opposed to the basic idea. That said, it's unfortunate that this needs to be raised to the AST level. Also, you have really been contributing to this project far to

[PATCH] D61280: Variable auto-init: don't initialize aggregate padding of all aggregates

rjmccall added a comment. I don't think the implication is supposed to be that padding is zero-initialized or not depending on where in the aggregate it appears, but it doesn't really matter, I don't think we're arguing about the goal of this patch. Repository: rC Clang CHANGES SINCE LAST A

[PATCH] D61304: [OpenCL][PR41609] Deduce static data members to __global addr space

rjmccall added a comment. Presumably a similar rule would apply to thread-locals if you supported them. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61304/new/ https://reviews.llvm.org/D61304 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D61274: [Sema][AST] Explicit visibility for OpenCL/CUDA kernels/variables

rjmccall added a comment. Okay. So it sounds like this should either be a device-only rule, with no warning in mixed-mode languages like CUDA, or we should take a different approach. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61274/new/ https://reviews.llvm

[PATCH] D61318: [Sema] Prevent binding references with mismatching address spaces to temporaries

rjmccall added inline comments. Comment at: lib/Sema/SemaInit.cpp:4836 + if (T1Quals.hasAddressSpace()) { +if (!T1Quals.isAddressSpaceSupersetOf(cv1T1IgnoreAS.getQualifiers())) { + Sequence.SetFailed( Isn't `cv1T1IgnoreAS.getQualifiers()` always `LangAS

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

rjmccall added a comment. Hmm. You know, there's another case where the destructor can be called even for a non-array: if constructing the object requires a temporary, I believe an exception thrown from that temporary's destructor is supposed to go back and destroy the variable. (This is, sad

[PATCH] D59924: [PowerPC] [Clang] Port MMX intrinsics and basic test cases to Power

rjmccall added a subscriber: chandlerc. rjmccall added a comment. @chandlerc, I hate to do this to you, but licensing question. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59924/new/ https://reviews.llvm.org/D59924 ___

[PATCH] D61304: [OpenCL][PR41609] Deduce static data members to __global addr space

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay. This seems reasonable to me, then. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61304/new/ https://reviews.llvm.org/D61304 _

[PATCH] D61318: [Sema] Prevent binding references with mismatching address spaces to temporaries

rjmccall added inline comments. Comment at: lib/Sema/SemaInit.cpp:4836 + if (T1Quals.hasAddressSpace()) { +if (!T1Quals.isAddressSpaceSupersetOf(cv1T1IgnoreAS.getQualifiers())) { + Sequence.SetFailed( Anastasia wrote: > rjmccall wrote: > > Isn't `cv1T1I

[PATCH] D58321: Support for relative vtables

rjmccall added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:531 +/// \brief Whether the class uses the relative C++ vtable ABI. +unsigned IsRelativeCXXABI : 1; + Should we proactively generalize this as a "CXXABIVariant" enum, which for

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

rjmccall added a comment. In D61165#1487100 , @erik.pilkington wrote: > It seems like the most common sense interpretation here is to just treat the > initialization of `G` as completed at the point when the constructor finishes > (this appears to be wh

[PATCH] D61454: [CodeGen][ObjC] Remove the leading 'l_' from ObjC symbols and make private symbols in the __DATA segment internal.

rjmccall added inline comments. Comment at: lib/CodeGen/CGObjCMac.cpp:1024 + bool AddToUsed, + bool ExplicitDataSegment); llvm::GlobalVariable *CreateMetadataVar(Twine Name, Plea

[PATCH] D60454: [OpenCL] Prevent mangling kernel functions

rjmccall added a comment. I think it would be more reasonable to just change `getDeclLanguageLinkage` to check for a kernel function. Comment at: lib/AST/Decl.cpp:2940 + if (hasAttr()) +return true; return isDeclExternC(*this); Both of these changes sh

[PATCH] D61318: [Sema] Prevent binding references with mismatching address spaces to temporaries

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:463 - /// Returns true if this address space is a superset of the other one. + /// Returns true if address space A is a superset of B. /// OpenCL v2.0 defines conversion rules (OpenCLC v2.0 s6.5.5) and no

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

rjmccall added a comment. In D61165#1488553 , @erik.pilkington wrote: > In D61165#1487328 , @rjmccall wrote: > > > In D61165#1487100 , > > @erik.pilkington wrote: > > > > >

[PATCH] D61454: [CodeGen][ObjC] Remove the leading 'l_' from ObjC symbols and make private symbols in the __DATA segment internal.

rjmccall added inline comments. Comment at: lib/CodeGen/CGObjCMac.cpp:3961 + // linkage so that the linker preserves the symbol name. + llvm::GlobalValue::LinkageTypes LT = ExplicitDataSegment || Section.empty() + ? llvm::GlobalValue::I

[PATCH] D61458: [hip] Relax CUDA call restriction within `decltype` context.

rjmccall added a comment. In D61458#1488972 , @hfinkel wrote: > In D61458#1488970 , @jlebar wrote: > > > Here's one for you: > > > > __host__ float bar(); > > __device__ int bar(); > > __host__ __device__ auto

[PATCH] D60454: [OpenCL] Prevent mangling kernel functions

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

[PATCH] D61318: [Sema] Prevent binding references with mismatching address spaces to temporaries

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

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

rjmccall added a comment. >>> That's only true for subobjects of an enclosing aggregate before that >>> aggregate's initialization is complete though, right? So it doesn't seem >>> like that much of an inconsistency, just mimicking what we would be doing >>> if an exception was thrown in, say,

[PATCH] D59988: [PR41276] Generate address space cast of 'this' for objects attributed by an address space in C++

rjmccall added inline comments. Comment at: test/CodeGenCXX/address-space-of-this.cpp:9 +//CHECK: call void @_ZN6MyTypeC1Ei(%struct.MyType* addrspacecast (%struct.MyType addrspace(10)* @m to %struct.MyType*), i32 123) +MyType __attribute__((address_space(10))) m = 123; -

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

rjmccall added a comment. In D61165#1490417 , @erik.pilkington wrote: > In D61165#1490168 , @rjmccall wrote: > > > I think the intuitive rule is that initialization is complete when the > > full-expression perform

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

rjmccall added a comment. The flip side of that argument is that (1) there aren't very many users right now and (2) it's much easier to start conservative and then weaken the rule than it will be to strengthen it later. It really isn't acceptable to just turn off access/use-checking for the de

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

rjmccall added a comment. In D61165#1492582 , @erik.pilkington wrote: > Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which > appears to claim that initialization ends with the execution of the > constructor, excluding temporary dest

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

rjmccall added a comment. In D61165#1492780 , @dexonsmith wrote: > In D61165#1492724 , @rjmccall wrote: > > > In D61165#1492582 , > > @erik.pilkington wrote: > > > > > Dunca

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

rjmccall added a comment. In D61165#1492781 , @rsmith wrote: > In D61165#1490417 , @erik.pilkington > wrote: > > > In D61165#1490168 , @rjmccall > > wrote: > > > > > I thin

[PATCH] D59988: [PR41276] Generate address space cast of 'this' for objects attributed by an address space in C++

rjmccall added inline comments. Comment at: test/CodeGenCXX/address-space-of-this.cpp:9 +//CHECK: call void @_ZN6MyTypeC1Ei(%struct.MyType* addrspacecast (%struct.MyType addrspace(10)* @m to %struct.MyType*), i32 123) +MyType __attribute__((address_space(10))) m = 123; -

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

rjmccall added a comment. In D61165#1494001 , @rsmith wrote: > In D61165#1492830 , @rjmccall wrote: > > > In D61165#1492781 , @rsmith wrote: > > > > > For the purposes of thi

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

rjmccall added a comment. All of the IRGen changes in this patch are unnecessary according to the model we've worked out, right? The fix is just to mark the destructor as still used when we're constructing an array. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13119 + // vari

[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; rsmith wrote: > Hmm,

[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: include/clang/Basic/AlignedExceptionObject.h:31 + case llvm::Triple::MacOSX: // Earliest supporting version is 10.14. +return llvm::VersionTuple(10U, 14U); + case llvm::Triple::IOS: ldionne wrote: > Would it make

[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: include/clang/Basic/AlignedExceptionObject.h:31 + case llvm::Triple::MacOSX: // Earliest supporting version is 10.14. +return llvm::VersionTuple(10U, 14U); + case llvm::Triple::IOS: rjmccall wrote: > ldionne wrote

[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: include/clang/Basic/DiagnosticGroups.td:411 def OpenCLUnsupportedRGBA: DiagGroup<"opencl-unsupported-rgba">; +def UnderalignedExcpObj : DiagGroup<"underaligned-exception-object">; def DeprecatedObjCIsaUsage : DiagGroup<"deprecated-obj

[PATCH] D51990: [DebugInfo] Fix emitting of bit offset for ObjC

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2369 + ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field) + : 0; } else { JDevlieghere wrote: > aprantl wrote: > > JDevlieghere wrote: > > >

[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

rjmccall added a comment. It might help if you're more specific about whose review you're asking for. Repository: rC Clang https://reviews.llvm.org/D50250 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailm

[PATCH] D51789: [clang] Add the exclude_from_explicit_instantiation attribute

rjmccall added a comment. That may work for libc++'s purposes, but it's clearly inappropriate as a compiler rule. There are good reasons why something with hidden visibility would need to be explicitly instantiated. For many programmers, hidden visibility means "this is private to my library"

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

rjmccall added inline comments. Comment at: lib/Analysis/ThreadSafetyCommon.cpp:362 + til::Project *P = new (Arena) til::Project(E, D); + if (hasCppPointerType(BE)) +P->setArrow(true); aaron.ballman wrote: > I feel like this will always return false. Howeve

[PATCH] D51789: [clang] Add the exclude_from_explicit_instantiation attribute

rjmccall added a comment. In https://reviews.llvm.org/D51789#1238410, @ldionne wrote: > In https://reviews.llvm.org/D51789#1238396, @rjmccall wrote: > > > That may work for libc++'s purposes, but it's clearly inappropriate as a > > compiler rule. There are good reasons why something with hidden

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

rjmccall added a comment. In https://reviews.llvm.org/D52200#1239007, @aaronpuchert wrote: > I think it should be possible to get rid of `self->` in the warning message > if we want to, after all `this->` is omitted in C++ as well. Hmm. It would be consistent to apply the same rule to both ca

[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)

rjmccall added a comment. Conceptually, this change looks great. And it should be fine to require extra alignment on `IdentifierInfo` on 32-bit hosts; I doubt that will have measurable impact. I believe it's possible to eliminate the need for most, perhaps all, of these `static_asserts` by ju

[PATCH] D52271: [Sema] Ensure that we retain __restrict qualifiers when substituting a reference type.

rjmccall added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:4268 +Quals.removeVolatile(); + } `restrict` is unique in being able to be applied to reference types. You should just rebuild `Quals` from scratch, preserving only `restrict`.

[PATCH] D52271: [Sema] Ensure that we retain __restrict qualifiers when substituting a reference type.

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

[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: include/clang/AST/DeclarationName.h:46 -/// DeclarationName - The name of a declaration. In the common case, -/// this just stores an IdentifierInfo poi

[PATCH] D52268: [AST] Squeeze some bits in LinkageComputer

rjmccall added a comment. `LinkageComputer` isn't actually persisted anywhere, right? And there's maybe one computer active at once? So this compression is theoretically saving one pointer of stack space but forcing a bunch of bit-manipulation every time these fields are accessed. Repositor

[PATCH] D52268: [AST] Squeeze some bits in LinkageComputer

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D52268#1241793, @riccibruno wrote: > In https://reviews.llvm.org/D52268#1241033, @rjmccall wrote: > > > `LinkageComputer` isn't actually persisted anywhere, righ

[PATCH] D52339: Support enums with a fixed underlying type in all language modes

rjmccall added a comment. In https://reviews.llvm.org/D52339#1242241, @aaron.ballman wrote: > In https://reviews.llvm.org/D52339#1242202, @erik.pilkington wrote: > > > From the last line in the paper, it seems that C++ compatibility is a goal > > of the paper (or at least a consideration). We sh

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenModule.cpp:4108 + // consistent with old behavior for this target as it would fail + // on the cast<> instead. + assert(GV && "isa isn't a GlobalValue"); compnerd wrote: > I think that

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

rjmccall added a reviewer: theraven. rjmccall added a comment. In https://reviews.llvm.org/D52344#1242451, @kristina wrote: > Would you be okay with me renaming `cfstring` to `.cfstring` for ELF so it's > in line with ELF section naming conventions (I'm not entirely sure if that > could cause i

[PATCH] D52440: Emit lifetime markers for temporary function parameter aggregates

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:176 + pushFullExprCleanup(NormalEHLifetimeMarker, + Slot.getAddress(), Size); +} This is problematic because we're not necessarily in a scope tha

[PATCH] D52598: [OpenCL] Fixed address space cast in C style cast of C++ parsing

rjmccall added inline comments. Comment at: lib/Sema/SemaCast.cpp:2283 + const QualType &SrcType, + const QualType &DestType) { + // OpenCL v1 s6.5: Casting a pointer to address space A to a pointer to ---

[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

rjmccall added a comment. Conceptually this seems fine, but I think it would be good to stop and make sure we're using a consistent style when mangling extensions. Currently it feels like every patch to add a Clang extension to the Microsoft mangling ends up inventing its own rules and crossin

[PATCH] D52738: [AST] Pack the bit-fields of FunctionProtoType into Type.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. The `reinterpret_cast` is legal because it's dynamic storage that's never actually accessed as an `ExceptionType`. IIRC there are reasonable-sounding strict-aliasing arguments about the v

[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

rjmccall added a comment. In https://reviews.llvm.org/D52674#1251439, @smeenai wrote: > In https://reviews.llvm.org/D52674#1251419, @rjmccall wrote: > > > Conceptually this seems fine, but I think it would be good to stop and make > > sure we're using a consistent style when mangling extensions.

[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

rjmccall added a comment. In https://reviews.llvm.org/D52674#1253401, @smeenai wrote: > Actually, I take that back ... I just misread the stack trace. > > There are a bunch of hops between the `mangleCXXRTTI` call and the ultimate > mangling function: > > MicrosoftMangleContextImpl::mangleCXXR

[PATCH] D52598: [OpenCL] Fixed address space cast in C style cast of C++ parsing

rjmccall added inline comments. Comment at: lib/Sema/SemaCast.cpp:2288 + SrcType->isPointerType()) { +const PointerType *DestPtr = DestType->getAs(); +if (!DestPtr->isAddressSpaceOverlapping(*SrcType->getAs())) { Anastasia wrote: > Anastasia wrote: >

[PATCH] D46441: [clang][CodeGenCXX] Noalias attr for copy/move constructor arguments

rjmccall added a comment. I'm fine with being more aggressive about this, and I agree that the standard should be making aliasing UB here. We use a similarly aggressive rule with return values: NRVO can allow direct access to the return slot, which we mark `noalias`, but which can in fact be a

[PATCH] D46441: [clang][CodeGenCXX] Noalias attr for copy/move constructor arguments

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1893 + +IsCtor = isa(TargetDecl); } I feel like you should just use `TargetDecl && isa(TargetDecl)` below; it's more obvious. Is there not an analogous rule for destructors? Repository:

[PATCH] D46441: [clang][CodeGenCXX] Noalias attr for 'this' parameter

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1893 + +IsCtor = isa(TargetDecl); } AntonBikineev wrote: > rjmccall wrote: > > I feel like you should just use `TargetDecl && > > isa(TargetDecl)` below; it's more obvious. > > > > Is the

[PATCH] D46441: [clang][CodeGenCXX] Noalias attr for 'this' parameter

rjmccall accepted this revision. rjmccall added a comment. LGTM. Comment at: lib/CodeGen/CGCall.cpp:2049 +// from the constructor’s this pointer, the value of the object or +// subobject thus obtained is unspecified. +unsigned ThisIRArg, NumIRArgs; -

[PATCH] D52598: [OpenCL] Fixed address space cast in C style cast of C++ parsing

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: lib/Sema/SemaCast.cpp:2288 + SrcType->isPointerType()) { +const PointerType *DestPtr = DestType->getAs(); +if (!DestPtr->isAddressSpaceOver

[PATCH] D57219: [Fixed Point Arithmetic] Fixed Point Comparisons

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3444 case BO_NE: +return Builder.CreateICmpNE(FullLHS, FullRHS); + case BO_Mul: Are padding bits guaranteed zero or unspecified? Or are we just not really supporting padding

[PATCH] D57219: [Fixed Point Arithmetic] Fixed Point Comparisons

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3444 case BO_NE: +return Builder.CreateICmpNE(FullLHS, FullRHS); + case BO_Mul: leonardchan wrote: > rjmccall wrote: > > Are padding bits guaranteed zero or unspecified? Or ar

[PATCH] D56555: Add Attribute to define nonlazy objc classes

rjmccall accepted this revision. rjmccall added a comment. Thanks! I'd like Aaron to sign off as well before we commit this. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56555/new/ https://reviews.llvm.org/D56555 _

<    3   4   5   6   7   8   9   10   11   12   >