[PATCH] D57438: [Sema][ObjC] Allow declaring ObjC pointer members in C++ unions under ARC

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:7084 + if (FD->getParent()->isUnion() && + shouldDeleteForVariantObjCPtrMember(FD, FieldType)) rjmccall wrote: > I believe the right check for variant-ness is `inUnion()`, not > `FD->ge

[PATCH] D57581: Explicitly add language standard option to test cases that rely on the C++14 default

rjmccall added a comment. I agree: if a test doesn't seem to require the GNU extensions, let's not use `-std=gnu++XX`. Otherwise this LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57581/new/ https://reviews.llvm.org/D57581 _

[PATCH] D57524: Fix ICE on attempt to add an addr space qual to a type qualified by an addr space

rjmccall added inline comments. Comment at: lib/Sema/SemaInit.cpp:4693 +T2Quals.addAddressSpace(AS2); + QualType WithAScv1T4 = S.Context.getQualifiedType(IgnoreAScv2T2, T1Quals); + Sequence.AddQualificationConversionStep(WithAScv1T4, ValueKind); ---

[PATCH] D57464: Generalize method overloading on addr spaces to C++

rjmccall added inline comments. Comment at: test/SemaCXX/address-space-method-overloading.cpp:28 + //inas4.bar(); + noas.bar(); // expected-error{{call to member function 'bar' is ambiguous}} +} ebevhan wrote: > Anastasia wrote: > > ebevhan wrote: > > > Just

[PATCH] D57464: Generalize method overloading on addr spaces to C++

rjmccall added inline comments. Comment at: test/SemaCXX/address-space-method-overloading.cpp:28 + //inas4.bar(); + noas.bar(); // expected-error{{call to member function 'bar' is ambiguous}} +} ebevhan wrote: > rjmccall wrote: > > ebevhan wrote: > > > Anasta

[PATCH] D57438: [Sema][ObjC] Allow declaring ObjC pointer members in C++ unions under ARC

rjmccall added a comment. Asking for a minor tweak, but feel free to commit with that. Comment at: lib/Sema/SemaDeclCXX.cpp:7084 + if (FD->getParent()->isUnion() && + shouldDeleteForVariantObjCPtrMember(FD, FieldType)) ahatanak wrote: > rjmccall wrote:

[PATCH] D57626: Disallow trivial_abi on a class if all copy and move constructors are deleted

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:7886 + if (!HasNonDeletedCopyOrMoveConstructor()) { +PrintDiagAndRemoveAttr(); +return; This is not a very useful diagnostic. We have several different reasons why we reject the attri

[PATCH] D57524: Fix ICE on attempt to add an addr space qual to a type qualified by an addr space

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

[PATCH] D57626: Disallow trivial_abi on a class if all copy and move constructors are deleted

rjmccall added a comment. Richard felt that this was an odd special case, and I was happy to use the old language-designer's dodge of banning something today so that we can decide to allow it tomorrow. This isn't SFINAE-able. ...of course, if it's just a *warning* that this isn't allowed, that

[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: > > leonardchan wrote: > > > rjmccall wrote: > > > > Are pa

[PATCH] D57797: Variable auto-init: fix __block initialization

rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1611 drillIntoBlockVariable(*this, Dst, &D); +} defaultInitNonTrivialCStructVar(Dst); Why don't you just initialize the payload right here, after we've drilled down to it? =

[PATCH] D57797: Variable auto-init: fix __block initialization

rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1611 drillIntoBlockVariable(*this, Dst, &D); +} defaultInitNonTrivialCStructVar(Dst); rjmccall wrote: > Why don't you just initialize the payload right here, after we've drilled >

[PATCH] D57464: Generalize method overloading on addr spaces to C++

rjmccall added a comment. Moving parsed attributes between lists isn't unreasonable if that's what you have to do; we already do that when processing the ObjC ARC qualifiers. The ambiguity with function attributes is pretty much inherent. Alternatively, you shouldn't feel like you can't impose

[PATCH] D57797: Variable auto-init: fix __block initialization

rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1643 CharUnits Size = getContext().getTypeSizeInChars(type); if (!Size.isZero()) { switch (trivialAutoVarInit) { jfb wrote: > rjmccall wrote: > > Can't you just drill to the byref

[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: ebevhan wrote: > ebevhan wrote: > > rjmccall wrote: > > > leonardchan wrote: > > > > ebevhan wro

[PATCH] D57768: [SYCL] Add SYCL device compilation flow.

rjmccall added a comment. In D57768#1386862 , @Anastasia wrote: > - SYCL seem to require adding tight dependencies from the standard libraries > into the compiler because many language features are hidden behind library > classes. This is not common for

[PATCH] D55659: [Sema][ObjC] Disallow non-trivial C struct fields in unions

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Two minor tweaks, but otherwise LGTM. Comment at: lib/AST/Type.cpp:2273 +const RecordDecl *RD = QT->castAs()->getDecl(); +// C++ classes are ignored. +if (isa

[PATCH] D57829: [HIP] Disable emitting llvm.linker.options in device compilation

rjmccall added a comment. Also, why is this HIP-specific? I thought the toolchain was largely shared with CUDA and there were just a few runtime differences. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57829/new/ https://reviews.llvm.org/D57829 ___

[PATCH] D57908: [SEMA]Generalize deferred diagnostic interface, NFC.

rjmccall added a comment. LGTM as well. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57908/new/ https://reviews.llvm.org/D57908 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/m

[PATCH] D57797: Variable auto-init: fix __block initialization

rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1605 -emitByrefStructureInit(emission); - // Initialize the variable here if it doesn't have a initializer and it is a Are these changes still needed? Comment at: lib/Cod

[PATCH] D57797: Variable auto-init: fix __block initialization

rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1726 +emitByrefStructureInit(emission); + } + jfb wrote: > Note that we still want this to be pulled out in this way because > `emitByrefStructureInit` emits the call to the initializer (in

[PATCH] D57797: Variable auto-init: fix __block initialization

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

[PATCH] D57936: [CodeGenObjC] When available, emit a direct call to objc_alloc_init(cls) instead of [objc_alloc(cls) init]

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:459 + llvm::Value *Receiver = + CGF.CGM.getObjCRuntime().GetClass(CGF, ObjTy->getInterface()); + return CGF.EmitObjCAllocInit(Receiver, CGF.ConvertType(OME->getType())); You might nee

[PATCH] D57918: Add an attribute that causes clang to emit fortified calls to C stdlib functions

rjmccall added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:986 + } + }]; +} Probably worth clarifying somewhere in here that only a specific set of stdlib functions are supported. I don't know that it's important to spell that set out.

[PATCH] D57464: Generalize method overloading on addr spaces to C++

rjmccall added a reviewer: rsmith. rjmccall added a comment. Hmm. Richard, I've mostly let you stay out of this, but do you have any thoughts about the right manage to parse attributes here? We want to allow `address_space` attributes to be written in the method-qualifiers list, but when we d

[PATCH] D57918: Add an attribute that causes clang to emit fortified calls to C stdlib functions

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, thanks! Comment at: clang/include/clang/Basic/AttrDocs.td:986 + } + }]; +} erik.pilkington wrote: > rjmccall wrote: > > Probably worth clarifying

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

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Ah, thanks, seems like a reasonable fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58016/new/ https://reviews.llvm.org/D58016 ___

[PATCH] D58061: Fix a few tests that were missing ':' on CHECK lines and weren't testing anything.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Good catch! Is there a reasonable way to just make FileCheck itself enforce this, or does that cause too many false positives with tests that are using e.g. `x86_64` as the entire prefix?

[PATCH] D58057: Allow bundle size to be 0 in clang-offload-bundler

rjmccall added a comment. I really don't understand this well enough to review. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58057/new/ https://reviews.llvm.org/D58057 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.l

[PATCH] D58060: Fix diagnostic for addr spaces in static_cast

rjmccall added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1830 + "binding value %diff{of type $ to reference to type $|to reference}0,1 " + "changes address space">; def err_reference_bind_failed : Error< We say that references are

[PATCH] D57978: [CodeGen] Generate follow-up metadata for loops with more than one transformation.

rjmccall resigned from this revision. rjmccall added a comment. Herald added a subscriber: jdoerfert. Sorry, this is deep in the loop analysis code, and I don't remember anything about this stuff, so I'm not sure I'm going to be a useful reviewer. Repository: rC Clang CHANGES SINCE LAST ACTI

[PATCH] D57898: CodeGen: Fix PR40605: split constant structures generated by -ftrivial-auto-var-init when emitting initializators

rjmccall added inline comments. Comment at: tools/clang/lib/CodeGen/CGDecl.cpp:979 + if (CGM.getCodeGenOpts().OptimizationLevel == 0) +return false; + if (GlobalSize <= SizeLimit) glider wrote: > jfb wrote: > > The general 64-byte heuristic is fine with me.

[PATCH] D57936: [CodeGenObjC] When available, emit a direct call to objc_alloc_init(cls) instead of [objc_alloc(cls) init]

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/CodeGen/CGObjC.cpp:475-483 case ObjCMessageExpr::Class: { ReceiverType = E->getClassReceiver(); const ObjCObjectType *ObjTy = Recei

[PATCH] D58218: Variable auto-init of blocks capturing self after init bugfix

rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1640 // Only initialize a __block's storage: we always initialize the header. -if (emission.IsEscapingByRef) +if (emission.IsEscapingByRef && isa(Loc.getPointer())) Loc = emitBlockByrefAddress(

[PATCH] D58145: [Sema] Fix a bogus -Wconversion warning

rjmccall added a comment. Basically LGTM, especially if we need an emergency fix, but please consider addressing my comment before committing since I'd expect it to be straightforward to solve. Comment at: clang/lib/Sema/SemaChecking.cpp:10634 + // warning. + else if (Resul

[PATCH] D58147: [CodeGen] Fix calling llvm.var.annotation outside of a basic block.

rjmccall added a comment. Okay. Is it acceptable for the annotation to simply disappear in this case? I don't really understand the purposes of annotations well enough to judge. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58147/new/ https://reviews.llvm.org/D58147 __

[PATCH] D58218: Variable auto-init of blocks capturing self after init bugfix

rjmccall added a comment. Well, you can always make a variable with a more directly-applicable name than `capturedByInit` and update it as appropriate, like `locIsByrefHeader`. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58218/new/ https://reviews.llvm.org/D58

[PATCH] D56411: [CUDA][HIP][Sema] Fix template kernel with function as template parameter

rjmccall added a comment. In D56411#1398097 , @yaxunl wrote: > In D56411#1365878 , @yaxunl wrote: > > > In D56411#1365745 , @rjmccall > > wrote: > > > > > In D56411#1365727

[PATCH] D58218: Variable auto-init of blocks capturing self after init bugfix

rjmccall added a comment. In D58218#1398124 , @jfb wrote: > In D58218#1398096 , @rjmccall wrote: > > > Well, you can always make a variable with a more directly-applicable name > > than `capturedByInit` and update

[PATCH] D56411: [CUDA][HIP][Sema] Fix template kernel with function as template parameter

rjmccall added a comment. In D56411#1398291 , @tra wrote: > >> That said, does CUDA have a general rule resolving `__host__` vs. > >> `__device__` overloads based on context? And does it allow overloading > >> based solely on `__host__` vs. `__device__`

[PATCH] D56411: [CUDA][HIP][Sema] Fix template kernel with function as template parameter

rjmccall added a comment. In D56411#1398328 , @rjmccall wrote: > In D56411#1398291 , @tra wrote: > > > >> That said, does CUDA have a general rule resolving `__host__` vs. > > >> `__device__` overloads based on con

[PATCH] D56411: [CUDA][HIP][Sema] Fix template kernel with function as template parameter

rjmccall added a comment. But what we've just been talking about is not a validity rule, it's an overload-resolution rule. It's not *invalid* to use a device function as a template argument to a host function template (or to a class template, which of course is neither host nor device). All y

[PATCH] D58218: Variable auto-init of blocks capturing self after init bugfix

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/D58218/new/ https://reviews.llvm.org/D58218 ___ cfe-commi

[PATCH] D58254: [Sema] Diagnose floating point conversions based on target semantics

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

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

rjmccall added inline comments. Comment at: lib/Sema/SemaCast.cpp:2306 + SrcPPointee.getAddressSpace()) || + !DestPPtr->isAddressSpaceOverlapping(*SrcPPtr)) { +Self.Diag(OpRange.getBegin(), This should `if (Nested ? DestPPoint

[PATCH] D56411: [CUDA][HIP][Sema] Fix template kernel with function as template parameter

rjmccall added a comment. It is totally unreasonable, at the time you are resolving a template argument, to investigate how the corresponding template parameter is used within the template and use that to shape how the template argument is resolved. That is simply not how the C++ template mode

[PATCH] D56411: [CUDA][HIP][Sema] Fix template kernel with function as template parameter

rjmccall added a comment. Okay, but it's not great design to have a kind of overloading that can't be resolved to an exact intended declaration even by an explicit cast. That's why I think making *optional* host/device typing is a good idea. And I strongly want to caution you against doing la

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

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

[PATCH] D58164: Block+lambda: allow reference capture

rjmccall added a comment. I agree. There should just never be a copy expression if the capture is of reference type, and there should therefore be no need to special-case that in IRGen. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58164/new/ https://reviews.l

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. The correctness condition here is solely that we cannot allow the block literal to go out of scope before the variable that it is assigned to. (Local block literals with captures have lifetimes like C compound literals: until the end of the enclosing block, rather tha

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. I see, alright. Comment at: lib/CodeGen/CGObjC.cpp:3215 +if (be->getBlockDecl()->canAvoidCopyToHeap()) + return value; + Can this just be a case in `ARCRetainExprEmitter`? I think that should subsume both this and the logic

[PATCH] D58346: [Sema] Change addr space diagnostics in casts to follow C++ style

rjmccall added a comment. One comment, but otherwise LGTM. Comment at: lib/Sema/SemaCast.cpp:2309 +auto DestPointeeTypeWithoutAS = Self.Context.removeAddrSpaceQualType( +DestPointeeType.getCanonicalType()); +return Self.Context.hasSameType(SrcPointeeTypeWithoutA

[PATCH] D36501: Add flag to request Clang is ABI-compatible with older versions of itself

rjmccall added a comment. The only thing I would say here is that, as a platform vendor, I would like the ability to opt in to ABI fixes that aren't in my base-line version. In particular, we're generally more aggressive about taking C++ ABI fixes on Darwin when there's a strong argument that

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

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3861 < Align.getQuantity()) || (ArgInfo.getIndirectByVal() && (RVAddrSpace != ArgAddrSpace))) { // Create an aligned temporary, and copy to it. yaxunl wrote

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

rjmccall added a comment. In https://reviews.llvm.org/D33826#856652, @lebedev.ri wrote: > In https://reviews.llvm.org/D33826#856619, @Eugene.Zelenko wrote: > > > In https://reviews.llvm.org/D33826#856610, @lebedev.ri wrote: > > > > > Any status update here? :) > > > I generally do see a benefit

[PATCH] D37042: Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:8808 +Context.getTypeSize(pointerType) == +Context.getTypeSize(IExp->getType())) + IsGNUIdiom = true; efriedma wrote: > andrew.w.kaylor wrote: > > efriedma wrote: > > > rs

[PATCH] D37310: [Atomic] Merge alignment information from Decl and from Type when emit atomic expression.

rjmccall added inline comments. Comment at: lib/CodeGen/CGAtomic.cpp:680 + alignChars = std::max(alignChars, alignDecl); +} + Just use EmitPointerWithAlignment instead of EmitScalarExpr to emit the pointer operand. Repository: rL LLVM https://revie

[PATCH] D37565: [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

rjmccall added inline comments. Comment at: lib/Sema/SemaChecking.cpp:8592 + bool BadR = !RType->isIntegerType() || RType->isSignedIntegerType() || + RHS->isKnownToHaveBooleanValue(); + Please extract a function which computes this for an Expr* and

[PATCH] D37565: [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

rjmccall added inline comments. Comment at: lib/Sema/SemaChecking.cpp:8879 + if (IsComparisonConstant) return AnalyzeImpConvsInComparison(S, E); lebedev.ri wrote: > rjmccall wrote: > > Part of the purpose of checking for signed comparisons up here is to

[PATCH] D37565: [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

rjmccall added inline comments. Comment at: lib/Sema/SemaChecking.cpp:8589 + Expr *LHS = E->getLHS()->IgnoreParenImpCasts(); + Expr *RHS = E->getRHS()->IgnoreParenImpCasts(); + lebedev.ri wrote: > rjmccall wrote: > > Do you still need these? I'm always antsy a

[PATCH] D37565: [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

rjmccall added a comment. Great, thanks. Just a few tweaks. Comment at: docs/ReleaseNotes.rst:76 + ``0`` constant was adjusted to warn regardless of whether the constant is + signed or unsigned. + "now warns when comparing an unsigned integer and 0 regardles

[PATCH] D37565: [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: docs/ReleaseNotes.rst:79 +- ``-Wtautological-compare`` now warns about comparison of signed integer and + ``0U`` constant when appropriate. + -

[PATCH] D37310: [Atomic] Merge alignment information from Decl and from Type when emit atomic expression.

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

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

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.h:211 + push_back(CallArg(rvalue, type, needscopy, AS)); } Ah, I think I understand what's going on here now. "indirect byval" arguments include an implicit copy to the stack, and the co

[PATCH] D33826: [clang-tidy] avoid pointer cast to more strict alignment check

rjmccall added a comment. In https://reviews.llvm.org/D33826#866170, @lebedev.ri wrote: > In https://reviews.llvm.org/D33826#866161, @JonasToth wrote: > > > There is an exception to the general rule (EXP36-C-EX2), stating that the > > result of `malloc` and friends is allowed to be casted to str

[PATCH] D32210: [Sema][ObjC] Add support for attribute "noescape"

rjmccall added a comment. Thanks, that looks good. https://reviews.llvm.org/D32210 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38118: [CodeGen][ObjC] Build the global block structure before emitting the body of global block invoke functions

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM with one tweak. Comment at: lib/CodeGen/CGBlocks.cpp:1124 LocalDeclMap, -

[PATCH] D38074: Fix TBAA information for reference accesses

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

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space of pointer/reference with template parameter types

rjmccall added a comment. I think the right approach here is probably to make sure you're applying deduction during instantiation as well. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62584/new/ https://reviews.llvm.org/D62584 ___ cfe-com

[PATCH] D62299: [PR41567][Sema] Fixed cast kind in addr space conversions

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Seems reasonable. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62299/new/ https://reviews.llvm.org/D62299 ___ cfe-commits mailing l

[PATCH] D58321: Support for relative vtables

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.def:329 +"Whether to use clang's relative C++ ABI " +"for classes with vtables") + leonardchan wrote: > rjmccall wrote: > > Yeah, see, this plays into the question a

[PATCH] D62643: [CodeGen][ObjC] Convert '[self alloc]' in a class method to 'objc_alloc(self)'

rjmccall added inline comments. Comment at: lib/CodeGen/CGObjC.cpp:376 + bool isClassMessage, + bool isSelfReceiverInClassMethod) { auto &CGM = CGF.CGM; I think it's fine to just call this a cla

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

rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11101 + if (auto *placeholderType = Init->getType()->getAsPlaceholderType()) +if (placeholderType->getKind() == BuiltinType::PseudoObject) { + Res = CheckPlaceholderExpr(Init).get(); -

[PATCH] D62708: Fix the predefined exponent limit macros for the 16-bit IEEE format.

rjmccall created this revision. rjmccall added a reviewer: scanon. Herald added subscribers: cfe-commits, Anastasia. Herald added a project: clang. Fix the predefined exponent limit macros for the 16-bit IEEE format. The magnitude range of normalized _Float16 is 2^-14 (~6e-5) to (2-2^-10)

[PATCH] D62708: Fix the predefined exponent limit macros for the 16-bit IEEE format.

rjmccall closed this revision. rjmccall added a comment. r362183 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62708/new/ https://reviews.llvm.org/D62708 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https:

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

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:8219 + // to allow skipping it. However we are not visiting all qual + // currently and therefore there is no condition yet. Diag(SL, diag::err_invalid_qualified_constructor) ---

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

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, LGTM. Comment at: test/SemaCXX/address-space-ctor.cpp:11 +//expected-note@-6{{candidate constructor (the implicit move constructor) not viable: no known conversion

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

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

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

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

[PATCH] D62988: Add an attribute to allow fields of non-trivial types in C unions

rjmccall added a reviewer: rsmith. rjmccall added a comment. Does this lead to C/C++ ABI mismatches? Should we just honor this in C++ as well by ignoring it when deciding to delete special members? Is taking such a general name a good idea if it's language-specific? Richard, thoughts? Repos

[PATCH] D59744: Fix i386 ABI "__m64" type bug

rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:1005 bool IsMCUABI; + bool IsLinuxABI; unsigned DefaultNumRegisterParameters; mgorny wrote: > Maybe replace the two booleans with something alike `IsPassInMMXRegABI`? And > while at it

[PATCH] D59744: Fix i386 ABI "__m64" type bug

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Minor comments, then LGTM. Comment at: lib/CodeGen/TargetInfo.cpp:1094 +// FreeBSD) don't want to spend any effort dealing with the ramifications +// of ABI break

[PATCH] D62988: Add an attribute to allow fields of non-trivial types in C unions

rjmccall added a comment. In D62988#1537401 , @ahatanak wrote: > In D62988#1537082 , @rjmccall wrote: > > > Does this lead to C/C++ ABI mismatches? Should we just honor this in C++ > > as well by ignoring it when

[PATCH] D62831: [CodeGen][ObjC] Add attribute "arc_retain_agnostic" to ObjC globals that are retain-agnostic

rjmccall added a comment. Should `objc` be in the attribute somewhere to avoid any future awkwardness? I don't remember what we've called similar attributes. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62831/new/ https://reviews.llvm.org/D62831 ___

[PATCH] D62988: Add an attribute to allow fields of non-trivial types in C unions

rjmccall added a comment. For ARC, you could bzero the union member; this is what how we tell people to initialize malloc'ed memory as well, since there's no default-constructor concept that they can invoke for such cases. Our immediate need for this attribute is that we have some code that wan

[PATCH] D63157: C++ DR712 and others: handle non-odr-use resulting from an lvalue-to-rvalue conversion applied to a member access or similar not-quite-trivial lvalue expression.

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:1429 +/// for instance if a block or lambda or a member of a local class uses a +/// const int variable or constexpr variable from an enclosing function. CodeGenFunction::ConstantEmission Isn't t

[PATCH] D63283: PR42182: Allow thread-local to use __cxa_thread_atexit when -fno-use-cxx-atexit is used

rjmccall added inline comments. Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2370 + if (CGM.getCodeGenOpts().CXAAtExit || D.getTLSKind()) return emitGlobalDtorWithCXAAtExit(CGF, dtor, addr, D.getTLSKind()); It's fine to match the GCC behavior, and it's

[PATCH] D62914: [Sema] Fix diagnostic for addr spaces in reference binding

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I like this wording of the diagnostic better in general. Could you make the diagnostic change in a separate patch and then fix the address-space problem? Both patches are approved. CHA

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

rjmccall added a comment. In what sense is the bit-pattern of a null pointer indeterminate? It's implementation-specified, but the compiler is certainly required to be able to produce that value during the constant initialization phase. If the language committee wants to make this non-constan

[PATCH] D63283: PR42182: Allow thread-local to use __cxa_thread_atexit when -fno-use-cxx-atexit is used

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

[PATCH] D63157: C++ DR712 and others: handle non-odr-use resulting from an lvalue-to-rvalue conversion applied to a member access or similar not-quite-trivial lvalue expression.

rjmccall added a comment. Minor comment request, then LGTM. Comment at: lib/CodeGen/CGDecl.cpp:1102 + // Form a simple per-variable LRU cache of these values in case we find we + // want to reuse them. + llvm::GlobalVariable *&CacheEntry = InitializerConstants[&D]; -

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

rjmccall added a comment. In D62825#1542301 , @rsmith wrote: > In D62825#1542247 , @rjmccall wrote: > > > In what sense is the bit-pattern of a null pointer indeterminate? > > > The problem is not null pointers, it'

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

rjmccall added a comment. In D62825#1542374 , @rsmith wrote: > In D62825#1542309 , @rjmccall wrote: > > > In D62825#1542301 , @rsmith wrote: > > > > > In D62825#1542247

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

rjmccall added a comment. In D62825#1542639 , @rsmith wrote: > In D62825#1542597 , @rjmccall wrote: > > > In D62825#1542374 , @rsmith wrote: > > > > > In D62825#1542309

[PATCH] D62831: [CodeGen][ObjC] Add attribute "objc_arc_intert" to ObjC globals that are retain-agnostic

rjmccall added a comment. Please rename the test. There's also a typo in the differential title; please make sure that doesn't get into the commit message. Otherwise LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62831/new/ https://reviews.llvm.org/D62831

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

rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:228 + /// Enable/Disable use of constrained floating point math + void setIsConstrainedFP(bool IsCon) { IsFPConstrained = IsCon; } + kpn wrote: > erichkeane wrote: > > kpn wrote: > > > kbar

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

rjmccall added a comment. Isn't `[[no_unique_address]]` only significant for empty members? I'm not sure why they need significant support from constant-building, since they expand to no meaningful initializer. We have some code we'll hopefully be upstreaming soon that relies on being able to

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

rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:113 +CR_ToZero ///< This corresponds to "fpround.tozero". + }; + kpn wrote: > rjmccall wrote: > > Should these have "FP" in the name somewhere? And are they really > > IRBuilder-

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

rjmccall added a comment. In D63371#1546587 , @rsmith wrote: > In D63371#1546500 , @rjmccall wrote: > > > Isn't `[[no_unique_address]]` only significant for empty members? I'm not > > sure why they need significan

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprConstant.cpp:73 +/// Incremental builder for an llvm::Constant* holding a structure constant. +class ConstantBuilder : private ConstantBuilderUtils { + llvm::SmallVector Elems; This seems like a very

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