[PATCH] D72411: [CodeGen] partially revert 2b4fa5348ee157b6b1a1af44d0137ca8c7a71573 to fix Objective-C static variable initialization

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:377 - if (D.needsDestruction(getContext()) && HaveInsertPoint()) { + if (hasNontrivialDestruction(D.getType()) && HaveInsertPoint()) { // We have a constant initializer, but a nontrivial destructor.

[PATCH] D72271: [Clang] Handle target-specific builtins returning aggregates.

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

[PATCH] D70172: [CUDA][HIP] Fix assertion due to dtor check on windows

rjmccall added a comment. In D70172#1812533 , @yaxunl wrote: > In D70172#1809571 , @rjmccall wrote: > > > I thought you were saying that the destructor decl hadn't been created yet, > > but I see now that you're sa

[PATCH] D70172: [CUDA][HIP] Fix assertion due to dtor check on windows

rjmccall added a comment. In D70172#1812664 , @yaxunl wrote: > In D70172#1812631 , @rjmccall wrote: > > > Most uses of the destructor do not use the delete operator, though, and > > therefore should not trigger the

[PATCH] D72411: [CodeGen] partially revert 2b4fa5348ee157b6b1a1af44d0137ca8c7a71573 to fix Objective-C static variable initialization

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:377 - if (D.needsDestruction(getContext()) && HaveInsertPoint()) { + if (hasNontrivialDestruction(D.getType()) && HaveInsertPoint()) { // We have a constant initializer, but a nontrivial destructor.

[PATCH] D71467: [FPEnv] Generate constrained FP comparisons from clang

rjmccall added a comment. Is this approach going to work with scope-local strictness? We need a way to do a comparison that has the non-strict properties but appears in a function that enables strictness elsewhere. Comment at: llvm/include/llvm/IR/IRBuilder.h:2342 r

[PATCH] D71467: [FPEnv] Generate constrained FP comparisons from clang

rjmccall added a comment. > Well, just like for all the other FP builder methods, you can use the > setIsFPConstrained method on the builder object to switch between strict and > non-strict mode. Does this not suffice, or is there anything particular about > the comparisons that would require a

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. In D75574#2262622 , @lanza wrote: >> I don't think it'll actually error out at link time: protocol objects get >> emitted eagerly on use, cross-module linking is just a code-size >> optimization. This actually has caused longsta

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

rjmccall added a comment. Also LGTM with a testcase. It's fine to test the result of IRGen. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87470/new/ https://reviews.llvm.org/D87470 ___ cfe-commits maili

[PATCH] D85960: [AST][FPEnv] Keep FP options in trailing storage of CastExpr

rjmccall added a comment. Follow-up on my previous request. It's fine by me if you commit with that fix. Comment at: clang/lib/Analysis/BodyFarm.cpp:188 + const_cast(Arg), nullptr, VK_RValue, + FPOptionsOverride

[PATCH] D85473: [Clang] Add option to allow marking pass-by-value args as noalias.

rjmccall added a reviewer: ahatanak. rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2198 +// reference to the underlying object. Mark it accordingly. +Attrs.addAttribute(llvm::Attribute::NoAlias); + fhahn wrote: > rjmccal

[PATCH] D86632: [Fixed Point] Add codegen for conversion between fixed-point and floating point.

rjmccall added inline comments. Comment at: llvm/include/llvm/IR/FixedPointBuilder.h:126 + /// \p Ty, or a floating point type with a larger exponent than Ty. + Type *getAccommodatingFloatType(Type *Ty, const FixedPointSemantics &Sema) { +const fltSemantics *FloatSema = &Ty

[PATCH] D85473: [Clang] Add option to allow marking pass-by-value args as noalias.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Just a minor tweak and then LGTM. Comment at: clang/include/clang/Driver/Options.td:4287-4290 +def fpass_by_value_noalias: Flag<["-"], "fpass-by-value-noalias">, + HelpT

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

rjmccall added a comment. In D87470#2268267 , @lxfind wrote: > hmm @rjmccall, I don't think there is a stable way to test this. The code > generated for symmetric transfer is way too complicated to stably pattern > match one less item in the frame. I do

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rjmccall added a comment. IRGen generally doesn't query the AST constant evaluator for arbitrary expressions; we do it for certain calls and loads because otherwise we might emit an illegal ODR-use, but otherwise we just expect LLVM's constant-folding to do a decent job. Repository: rG LLVM

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

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

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rjmccall added a comment. Richard, what do you think the right rule is for when to use the special constant-evaluation rules for this feature in C++? The C standard says that constant expressions should use the default rules rather than considering the dynamic environment, but C can get away w

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rjmccall added a subscriber: scanon. rjmccall added a comment. That's a really useful concept, and given that it exists, I agree that we shouldn't invent something else. The fact that it covers local variables with constant initializers that happen to be `const` seems really unfortunate, thoug

[PATCH] D87761: [clang][codegen] Skip adding default function attributes on intrinsics.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Seems reasonable. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87761/new/ https://reviews.llvm.org/D87761

[PATCH] D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1

rjmccall added a comment. In D80344#2280617 , @tentzen wrote: > Hi, John, thank you for reviewing this patch and providing feedback. > regarding your comments: > > 1, In the RFC thread early, Reid K (the major contributor of Windows SEH/EH > support) had

[PATCH] D87822: [FPEnv] Evaluate constant expressions under non-default rounding modes

rjmccall added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:2427 +FPOptions FPFeatures = Cast->getFPFeaturesInEffect(Info.Ctx.getLangOpts()); +RM = FPFeatures.getRoundingMode(); + } I think the options really need to be passed in or else c

[PATCH] D87917: [Sema] Handle objc_super special lookup when checking builtin compatibility

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:9674 ASTContext::GetBuiltinTypeError Error; +LookupPredefedObjCSuperType(*this, S, NewFD->getIdentifier()); QualType BuiltinType = Context.GetBuiltinType(BuiltinID, Error

[PATCH] D87822: [FPEnv] Evaluate constant expressions under non-default rounding modes

rjmccall added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:2462 + return true; +} + Thanks, these look good. Comment at: clang/lib/AST/ExprConstant.cpp:2427 +FPOptions FPFeatures = Cast->getFPFeaturesInEffect(Info.Ctx.getLa

[PATCH] D87917: [Sema] Handle objc_super special lookup when checking builtin compatibility

rjmccall added a comment. Oh, I didn't notice that this only did work conditionally based on the builtin ID. Yes, please make a function like `lookupNecessaryTypesForBuiltin` that takes the builtin ID. Maybe we can generalize this to solve the problem with that other builtin, too. Repositor

[PATCH] D87983: [Sema] Split special builtin type lookups into a separate function

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks. Minor improvement and then LGTM. Comment at: clang/lib/Sema/SemaLookup.cpp:924 +void Sema::LookupNecessaryTypesForBuiltin(Scope *S, unsigned ID) { + if (getLang

[PATCH] D86632: [Fixed Point] Add codegen for conversion between fixed-point and floating point.

rjmccall added a comment. I had a question in the other patch about whether you should just have a method on FixedPointSemantics that returns the unscaled semantics (since FixedPointSemantics is totally capable of expressing integer types), which would let fitsInFloatSemantics have more obvious

[PATCH] D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1

rjmccall added a comment. In D80344#228 , @tentzen wrote: > Thank you for prompt reply again. > >> [rjmccall] And I agree with him that the potential benefits are substantial. >> I'm just saying that one barely-trafficked RFC thread is not evidence of

[PATCH] D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1

rjmccall added a comment. In D80344#2288898 , @tentzen wrote: > In D80344#2286838 , @rjmccall wrote: > >> In D80344#228 , @tentzen wrote: >> >>> There is absolutely NO ex

[PATCH] D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1

rjmccall added a comment. In D80344#2291385 , @asmith wrote: >> @rjmccall wrote: >> I think you're missing what I'm asking. If LLVM accepts this feature, it >> will become our collective responsibility as a project to keep it working. >> You have a la

[PATCH] D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1

rjmccall added a comment. In D80344#2291456 , @tentzen wrote: > In D80344#2291156 , @rjmccall wrote: > >> In D80344#2288898 , @tentzen wrote: >> >>> In D80344#2286838

[PATCH] D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1

rjmccall added a comment. Okay. I think we're on the same page about representation now. If you can come up with a good replacement for "eha" in the intrinsic names, I think this is pretty much ready to go. Comment at: llvm/docs/LangRef.rst:11584 +These intrinsics make Bloc

[PATCH] D87822: [FPEnv] Evaluate constant expressions under non-default rounding modes

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/AST/ExprConstant.cpp:2427 +FPOptions FPFeatures = Cast->getFPFeaturesInEffect(Info.Ctx.getLangOpts()); +RM = FPFeatures.getRoundingMode(

[PATCH] D88236: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:2148 +CommonElementType == nullptr && !NumInitElts) { const ArrayType *AT = CGM.getContext().getAsArrayType(DestType); CommonElementType = CGM.getTypes().ConvertType(AT->getElem

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. Okay, the result of internal review is that we're comfortable with this feature. Reviewers brought up the point that it would be interesting to add some way to ensure unique emission of a protocol, so that protocols that can't be non-runtime could avoid bloating binary

[PATCH] D88236: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes

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

[PATCH] D88236: [PR47636] Fix tryEmitPrivate to handle non-constantarraytypes

rjmccall added a comment. Yeah, that would also be a completely reasonable approach. We already preserve the source spelling of the incomplete array type in the appropriate expression. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88236/new/ https://reviews.llvm.org/D88236 __

[PATCH] D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1

rjmccall added a comment. In D80344#2293954 , @tentzen wrote: > In D80344#2291926 , @rjmccall wrote: > >> Okay. I think we're on the same page about representation now. If you can >> come up with a good replaceme

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:490 + llvm::UniqueVector FoundProtocols; + std::set DeclaredProtocols; + You should use llvm::DenseSet for this. But in general there are more sets here than I think you really need, and

[PATCH] D88329: [objc] Consolidate ObjC name mangle code to AST

rjmccall added inline comments. Comment at: clang/include/clang/AST/Mangle.h:130 + bool includeCategoryNamespace = true); void mangleObjCMethodName(const ObjCMethodDecl *MD, raw_ostream &); Could you switch the polarity

[PATCH] D88329: [objc] Consolidate ObjC name mangle code to AST

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

rjmccall added a comment. I think that, if we want to do this, we need to think carefully about what exactly we want the ABI to be. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86049/new/ https://reviews.llvm.org/D86049 _

[PATCH] D86218: Teach the swift calling convention about _Atomic types

rjmccall added a comment. Please do this as a very late check rather than the first check. You need to check for extra atomic padding. If there's any difference between the sizes of the atomic type and its element, just add it as opaque data. Repository: rG LLVM Github Monorepo CHANGES SIN

[PATCH] D86218: Teach the swift calling convention about _Atomic types

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

[PATCH] D86668: Fix Calling Convention of __float128 and long double(128bits) in i386

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:1603 + if (IsLinuxABI && Ty->isFloatingType() && getContext().getTypeSize(Ty) == 128) +return 16; + I don't think this should be restricted to just Linux. It's a fix for all OSes

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

rjmccall added a comment. I'd still like @rsmith to sign off here as code owner. Comment at: clang/include/clang/Basic/IdentifierTable.h:231 return ObjCOrBuiltinID == tok::NUM_OBJC_KEYWORDS; } Do we need to support reverting builtins anymore? ==

[PATCH] D86854: [CodeGen] Make sure the EH cleanup for block captures is conditional when the block literal is in a conditional context

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM modulo the minor fix Comment at: clang/lib/CodeGen/CGDecl.cpp:2107 +// FIXME: When popping normal cleanups, we need to keep this EH cleanup +// around in cas

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

rjmccall added inline comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:3387 Bits = (Bits << 1) | unsigned(II->isPoisoned()); -Bits = (Bits << 1) | unsigned(II->hasRevertedBuiltin()); +Bits <<= 1; // Previously used to indicate reverted builtin. Bits =

[PATCH] D86980: [Docs] Remove outdated OS limitation

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

[PATCH] D86993: Document Clang's expectations of the C standard library.

rjmccall added a comment. Wording looks good. Should we alsso document our assumptions about what functions exist in the standard library — the functions that we'll always use even in freestanding builds? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

rjmccall added a comment. The builtins with custom type-checking are all true intrinsics like `__builtin_operator_new` and so on. They really can't be validly declared by the user program. The thing that seems most likely to avoid random compiler crashes would be to either forbid explicit dec

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

rjmccall added a comment. I didn't see the specific example, sorry. I agree that my description is more applicable to builtins in the `__builtin` namespace, which describes most of the builtins with custom typechecking. Is the problem just `__va_start`? If we have to treat all declarations as

[PATCH] D86993: Document Clang's expectations of the C standard library.

rjmccall added a subscriber: t.p.northover. rjmccall added a comment. @t.p.northover says it's complicated. `memcpy`, `memmove`, `memset`, and `bzero` are (I think) the only ones that LLVM will potentially synthesize from nothing and therefore need to be present even in freestanding builds; tha

[PATCH] D87102: [Sema] Fix a -Warc-repeated-use-of-weak false-positive by only calling CheckPlaceholderExpr once

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/D87102/new/ https://reviews.llvm.org/D87102 ___ cfe-commits mailing list cfe-commits

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

rjmccall added a comment. We just talk about it. I agree with Nathan that we shouldn't just add this as a short-term hack; we should design the ABI right and then do what we want. I think these are basically all the ABI questions: - Is there a good reason to preserve signature compatibility wi

[PATCH] D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1

rjmccall added a comment. I've made some brief comments about the code, but I think I have much larger concerns here. The first is whether LLVM really intends to satisfy the constraints necessary to make these exceptions work, which I don't think you've gotten clear consensus about at all. Un

[PATCH] D86921: [FPEnv] Partially implement #pragma STDC FENV_ROUND

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

[PATCH] D85473: [Clang] Add option to allow marking pass-by-value args as noalias.

rjmccall added a comment. Is it acceptable to leave this as a -cc1 option while we're pursuing this with the language committee? Do we have any intent to pursue this with the language committee? Comment at: clang/include/clang/Basic/LangOptions.def:372 +LANGOPT(PassByValueIs

[PATCH] D85960: [AST][FPEnv] Keep FP options in trailing storage of CastExpr

rjmccall added a comment. Minor request, but otherwise LGTM. Comment at: clang/lib/Analysis/BodyFarm.cpp:188 + const_cast(Arg), nullptr, VK_RValue, + FPOptionsOverride()); } Can these call sites

[PATCH] D87331: Sema: add support for `__attribute__((__swift_error__))`

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

[PATCH] D87331: Sema: add support for `__attribute__((__swift_error__))`

rjmccall added a comment. Oh, there are a ton of typos in this documentation. Comment at: clang/include/clang/Basic/AttrDocs.td:3396 +the imported API, and dynamically will always pass a valid address initialized +to a null pointer. + Please split this sentence

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. I don't think it'll actually error out at link time: protocol objects get emitted eagerly on use, cross-module linking is just a code-size optimization. This actually has caused longstanding problems. Anyway, I think it's fine to require forward declarations to have t

[PATCH] D87331: Sema: add support for `__attribute__((__swift_error__))`

rjmccall accepted this revision. rjmccall added a comment. LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87331/new/ https://reviews.llvm.org/D87331 ___ cfe-commits mailing list cfe-commits@

[PATCH] D87331: Sema: add support for `__attribute__((__swift_error__))`

rjmccall added inline comments. Comment at: clang/include/clang/Basic/Attr.td:2128 + ]; + let Subjects = SubjectList<[Function, ObjCMethod], ErrorDiag>; + let Documentation = [SwiftErrorDocs]; compnerd wrote: > aaron.ballman wrote: > > Should this apply to fun

[PATCH] D87331: Sema: add support for `__attribute__((__swift_error__))`

rjmccall added a comment. `const` qualification isn't the most meaningful thing for Objective-C objects anyway. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87331/new/ https://reviews.llvm.org/D87331 _

[PATCH] D92808: [ObjC][ARC] Annotate calls with attributes instead of emitting retainRV or claimRV calls in the IR

rjmccall added a comment. Your patch description doesn't mention the changes to the inliner. Comment at: clang/lib/CodeGen/CGObjC.cpp:2282 +static const char *markerKey = "clang.arc.retainAutoreleasedReturnValueMarker"; + This is too generic for a global vari

[PATCH] D91488: Consider reference, pointer, and pointer-to-membber TemplateArguments to be different if they have different types.

rjmccall added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:3758 TemplateArgument TA(T->getNumBitsExpr()); - mangleTemplateArgs(&TA, 1); + mangleTemplateArgs(TemplateName(), &TA, 1); if (T->isUnsigned()) Passing a null TemplateName here ca

[PATCH] D91488: Consider reference, pointer, and pointer-to-membber TemplateArguments to be different if they have different types.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Seems fine to me. Comment at: clang/lib/AST/ItaniumMangle.cpp:4847 + /// Do we need to mangle template arguments with exactly correct types? + bool needExactType(unsign

[PATCH] D92577: Don't reject tag declarations in for loop clause-1

rjmccall added a comment. Please test that there's actually an object declared and that it's not *just* a tag declaration. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92577/new/ https://reviews.llvm.org/D92577 ___ cfe-commits mailing list

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

rjmccall added a comment. I think that as long as the class leaves a copy/move constructor defaulted, there's no need for a new trivial_abi attribute. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92361/new/ https://reviews.llvm.org/D92361 __

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

rjmccall added a comment. In D92361#2459513 , @zoecarver wrote: >> I think that as long as the class leaves a copy/move constructor defaulted, >> there's no need for a new trivial_abi attribute. > > Sorry, I'm not sure I follow. Could you elaborate a bit

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

rjmccall added a comment. Oh yes, somehow I overlooked that, sorry. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92361/new/ https://reviews.llvm.org/D92361 ___ cfe-commits mailing list cfe-commits@lists

[PATCH] D92577: Don't reject tag declarations in for loop clause-1

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:1846 +(*Iter)->setInvalidDecl(); + } } You can have multiple tag declarations because of complex declarators or type-specifiers, e.g. `struct A (*var)(struct B)

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

rjmccall added a comment. `byval` actually means it'll be passed on the stack, not as a pointer, so yes, that's a real change in conventions from an indirect argument. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92361/new/ https://reviews.llvm.o

[PATCH] D92577: Don't reject tag declarations in for loop clause-1

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/D92577/new/ https://reviews.llvm.org/D92577 ___ cfe-commits mailing list cfe-commits

[PATCH] D93273: [CodeGen][ObjC] Destroy callee-destroyed arguments in the caller function when the receiver is nil

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Functionally LGTM. Minor suggestion. Comment at: clang/lib/CodeGen/CGObjCMac.cpp:2169 + return false; +} + Is this something that could reasonably just

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added a comment. Okay. It sounds like strict compatibility with GCC implies ignoring pragmas in fast, and that's what we're most concerned with, since this is originally a GCC option. So the only question I have now is whether `faststd` is the best name for this. Does anyone want to

[PATCH] D89998: [c++20] For P0732R2 / P1907R1: Basic code generation and name mangling supportfor non-type template parameters of class type and template parameter objects.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/AST/ItaniumMangle.cpp:5136 + case APValue::FixedPoint: +llvm_unreachable("Fixed point types are disabled for c++"); +return; -

[PATCH] D91111: [CodeGen] Mark calls to objc_autorelease as tail

rjmccall added a comment. Alright. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D9/new/ https://reviews.llvm.org/D9 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/

[PATCH] D90434: [CodeGen] Correct codegen for self-capturing __block var

rjmccall added a comment. This is great work, thank you. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5349 +def note_because_captured_by_block : Note< + "because it is captured by a block used in its own initializer">; + This will read okay on

[PATCH] D90434: [CodeGen] Correct codegen for self-capturing __block var

rjmccall added inline comments. Comment at: clang/lib/AST/Decl.cpp:2491 +bool VarDecl::isCapturedByOwnInit() const { + return hasAttr() && NonParmVarDeclBits.CapturedByOwnInit; +} ille wrote: > rjmccall wrote: > > You should check `isEscapingByref()` here rather

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added a comment. `fast-strict`? Sounds sort of oxymoronic, but it's `fast` while also being strict about honoring pragmas. I don't have any great ideas here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 ___

[PATCH] D91253: [Matrix] Update mangling to use paramterized vendor ext type syntax.

rjmccall added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:3497 // Mangle matrix types using a vendor extended type qualifier: - // Umatrix_type + // umatrix_typeIE StringRef VendorQualifier = "matrix_type"; "Mangle matrix types as a vend

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added a comment. Probably should be pluralized for consistency, `fast-honor-pragmas`, but yeah, that's fine with me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 ___ cfe-commits mailing li

[PATCH] D91253: [Matrix] Update mangling to use paramterized vendor ext type syntax.

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

[PATCH] D90622: clang: Don't assert on no_unique_address fields in @encode()

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/D90622/new/ https://reviews.llvm.org/D90622 ___ cfe-commits mailing list cfe-commits

[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2169 +if (!CodeGenOpts.NullPointerIsValid && +getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0) { + Attrs.addAttribute(llvm::Attribute::NonNull); rsmith wrote:

[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2169 +if (!CodeGenOpts.NullPointerIsValid && +getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0) { + Attrs.addAttribute(llvm::Attribute::NonNull); rsmith wrote:

[PATCH] D91269: [Clang][CodeGen][RISCV] Add hard float ABI tests with empty struct

rjmccall accepted this revision. rjmccall added a comment. We don't usually add known-broken tests like this before a fix, as opposed to just landing them as part of the fix, but if you have a good reason to do so it's okay. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2169 +if (!CodeGenOpts.NullPointerIsValid && +getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0) { + Attrs.addAttribute(llvm::Attribute::NonNull); arichardson wr

[PATCH] D91596: [PowerPC] [Clang] Fix alignment of 128-bit floating types

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I assume this has always been taken care of properly in the backend, so this is just a fix for va_arg treatment? If so, LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91596/

[PATCH] D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1

rjmccall added a comment. Sounds good to me, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80344/new/ https://reviews.llvm.org/D80344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https

[PATCH] D88329: [objc] Consolidate ObjC name mangle code to AST

rjmccall added a comment. Done as 98ef7e29b0fe03da77fa6ef5c86bea9e31c178d0 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88329/new/ https://reviews.llvm.org/D88329

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

rjmccall added a comment. Being permissive about recognizing builtins when the expected signature requires a type that lookup can't find seems completely reasonable. We don't really want to force library functions to take the custom-typechecking path just because we want to infer an attribute

[PATCH] D88497: [objc] Fix memory leak in CGObjCMac.cpp

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Oh, oops, I should've caught this in review. I assume you still need a commit? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88497/new/ htt

[PATCH] D87822: [FPEnv] Evaluate constant expressions under non-default rounding modes

rjmccall added a comment. Commented in that review, but that patch has the wrong fix: it's based on the targets LLVM is configured with rather than the current test target. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87822/new/ https://reviews.l

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rjmccall added a comment. Richard and I had a long conversation about this further up-thread, if you missed it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87528/new/ https://reviews.llvm.org/D87528 ___ cfe-commits mailing list cfe-commits

[PATCH] D88518: Recognize setjmp and friends as builtins even if jmp_buf is not declared yet.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. I like this better as a fix for PR40692 anyway. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88518/new/ https://reviews.llvm.org/D88

[PATCH] D88497: [objc] Fix memory leak in CGObjCMac.cpp

rjmccall added a comment. Committed (with substantial changes) as 984744a1314ce165378e7945bc45995302a8cb80 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88497/new/ https://revi

[PATCH] D88664: [AST] do not error on APFloat invalidOp in default mode

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/D88664/new/ https://reviews.llvm.org/D88664 ___ cfe-commits mailing list cfe-commit

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. Thanks, that's a lot better. Just some minor suggestions. Comment at: clang/lib/CodeGen/CGObjC.cpp:477 + // If there are no non-runtime protocols then we can just stop now. + if (!NonRuntimePDs.size()) +return RuntimePds; `empty

<    14   15   16   17   18   19   20   21   22   23   >