[PATCH] D55869: Convert some ObjC retain/release msgSends to runtime calls

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. It sounds like it's fine. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55869/new/ https://reviews.llvm.org/D55869 ___ cfe-commits m

[PATCH] D56022: [AST] Store the arguments of CXXConstructExpr in a trailing array

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: include/clang/AST/ExprCXX.h:1613 + + TypeSourceInfo *TSI = nullptr; + riccibruno wrote: > Changed the name from `Type` since `Type` wa

[PATCH] D55948: Modify DeclaratorChuck::getFunction to use DeclSpec for qualifiers

rjmccall added inline comments. Comment at: include/clang/Sema/DeclSpec.h:1365 +AttributeFactory AttrFactory; +MethodQualifiers = new DeclSpec(AttrFactory); + } This `DeclSpec` ends up with a dangling reference to the `AttributeFactory`; I t

[PATCH] D56066: [OpenCL] Address space for default class members

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:78 +RecTy = Context.getAddrSpaceQualType( +RecTy, MD->getTypeQualifiers().getAddressSpace()); return Context.getPointerType(CanQualType::CreateUnsafe(RecTy)); Would you mind making

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

rjmccall added inline comments. Comment at: lib/Sema/SemaOverload.cpp:9279 +(CandAS1 != LangAS::opencl_generic && CandAS1 != LangAS::Default)) + return true; + } Anastasia wrote: > rjmccall wrote: > > rjmccall wrote: > > > This at least needs a comm

[PATCH] D56134: [AST] Store some data of CXXNewExpr as trailing objects

rjmccall added a comment. LGTM. Comment at: include/clang/AST/ExprCXX.h:1950 /// The allocated type-source information, as written in the source. TypeSourceInfo *AllocatedTypeInfo; If you ever want a somewhat bigger refactoring project that might provid

[PATCH] D56188: Adopt SwiftABIInfo for WebAssembly.

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

[PATCH] D56188: Adopt SwiftABIInfo for WebAssembly.

rjmccall added a comment. ...although it might be reasonable to extract the method implementations on `DefaultABIInfo` as helper functions so that the code can be reused without requiring a particular inheritance hierarchy. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.ll

[PATCH] D56198: [Basic] Extend DiagnosticEngine to store and format Qualifiers

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:6705 +/// Insertion operator for diagnostics. This allows sending Qualifiers' into a +/// diagnostic with <<. +inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB, Unpaired ap

[PATCH] D55948: Modify DeclaratorChuck::getFunction to use DeclSpec for qualifiers

rjmccall added inline comments. Comment at: lib/Parse/ParseDeclCXX.cpp:2368 } }; + DeclSpecCheck(DeclSpec::TQ_const, "const", DS.getConstSpecLoc()); Anastasia wrote: > rjmccall wrote: > > Please remove this dead semicolon since you're touchin

[PATCH] D55948: Modify DeclaratorChuck::getFunction to use DeclSpec for qualifiers

rjmccall added inline comments. Comment at: lib/Sema/DeclSpec.cpp:220 + + I.Fun.QualAttrFactory = nullptr; + Anastasia wrote: > rjmccall wrote: > > Spacing? > I am keeping the old formatting from the lines 195-204. But obviously it > doesn't match with

[PATCH] D56198: [Basic] Extend DiagnosticEngine to store and format Qualifiers

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

[PATCH] D56066: [OpenCL] Address space for default class members

rjmccall added inline comments. Comment at: lib/Sema/SemaInit.cpp:4539 + if (InitCategory.isPRValue() || InitCategory.isXValue()) +T1Quals.removeAddressSpace(); + rjmccall wrote: > I can understand why a pr-value wouldn't have an address space, but an > x-v

[PATCH] D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC

rjmccall added inline comments. Comment at: clang/docs/AutomaticReferenceCounting.rst:1748 +``objc_externally_retained`` can apply to local variables, or to parameters via +a function definitions. + I think "can apply to parameters and local variables" is probabl

[PATCH] D55948: Modify DeclaratorChuck::getFunction to use DeclSpec for qualifiers

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:8175 DeclaratorChunk::FunctionTypeInfo &FTI = D.getFunctionTypeInfo(); - if (FTI.TypeQuals != 0) { -if (FTI.TypeQuals & Qualifiers::Const) - Diag(D.getIdentifierLoc(), diag::err_invalid_qualified_co

[PATCH] D56188: Adopt SwiftABIInfo for WebAssembly.

rjmccall added a comment. I don't mind you committing it like this. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56188/new/ https://reviews.llvm.org/D56188 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

[PATCH] D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC

rjmccall added inline comments. Comment at: clang/docs/AutomaticReferenceCounting.rst:1749 +instance, capturing an externally-retained variable in a block will retain the +value when capturing it, and release it when the block is destroyed. + Nit-picks: `it's` sh

[PATCH] D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC

rjmccall added a comment. Sorry, I keep coming up with small tweaks to the documentation. These should be the last of them, so if Aaron's also satisfied, feel free to commit with those modifications. Comment at: clang/docs/AutomaticReferenceCounting.rst:1788 +// ... + }

[PATCH] D55662: [Sema][ObjC] Do not warn about repeated uses of weak variables when the variables are accessed in an unevaluated context.

rjmccall added a comment. I think you could just disable the diagnostic in an unevaluated context. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55662/new/ https://reviews.llvm.org/D55662 ___ cfe-commits mailing list

[PATCH] D55662: [Sema][ObjC] Do not warn about repeated uses of weak variables when the variables are accessed in an unevaluated context.

rjmccall added a comment. In D55662#1346059 , @ahatanak wrote: > In D55662#1346055 , @rjmccall wrote: > > > I think you could just disable the diagnostic in an unevaluated context. > > > The call to `isUnevaluatedCo

[PATCH] D55948: Modify DeclaratorChuck::getFunction to use DeclSpec for qualifiers

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:8175 DeclaratorChunk::FunctionTypeInfo &FTI = D.getFunctionTypeInfo(); - if (FTI.TypeQuals != 0) { -if (FTI.TypeQuals & Qualifiers::Const) - Diag(D.getIdentifierLoc(), diag::err_invalid_qualified_co

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. What's the general idea here, that you're going to pretend to be the environment's "standard" CPU target of the right pointer width and try to match the ABI exactly? This seems like a pretty treacherous road to go down. CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D56225: [HIP] Use nul instead of /dev/null when running on windows

rjmccall added inline comments. Comment at: lib/Driver/ToolChains/HIP.cpp:210 std::string BundlerTargetArg = "-targets=host-x86_64-unknown-linux"; - std::string BundlerInputArg = "-inputs=/dev/null"; + std::string BundlerInputArg = "-inputs=" NULL_FILE; Ho

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. No, no, I understand that you're not changing pointer sizes, but this is one example of trying to match the ABI of the target environment, and I'm trying to understand how far that goes. What does it mean to be in the "MSVC" environment when you're actually just compi

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. Okay. Is there a reasonable way to make your targets delegate to a different `TargetInfo` implementation for most things so that you can generally match the host target for things like type sizes and alignments? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D5

[PATCH] D55662: [Sema][ObjC] Do not warn about repeated uses of weak variables when the variables are accessed in an unevaluated context.

rjmccall added a comment. Can we find a way to preserve the rewritten expression out of `DeduceAutoType` so that the initialization code just happens to never see a placeholder along this path? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55662/new/ https://re

[PATCH] D56358: [AST] Move back BasePathSize to the bit-fields of CastExpr

rjmccall added inline comments. Comment at: include/clang/AST/Expr.h:3029 +assert((CastExprBits.BasePathSize == BasePathSize) && + "BasePathSize overflow!"); assert(CastConsistency()); riccibruno wrote: > lebedev.ri wrote: > > riccibruno wrote:

[PATCH] D56367: [AST] Pack CXXDependentScopeMemberExpr

rjmccall added a comment. Does the regression disappear if you make the scope specifier the first trailing object? Later trailing objects are more expensive to access, and I would imagine that the scope specifier and template arguments are more important than the other fields. Repository:

[PATCH] D56368: [AST] Store the results in OverloadExpr in a trailing array

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

[PATCH] D56367: [AST] Pack CXXDependentScopeMemberExpr

rjmccall added a comment. Okay. That comment seems reasonable. Glad to hear you're on top of it. :) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56367/new/ https://reviews.llvm.org/D56367 ___ cfe-commits mailing l

[PATCH] D56066: [OpenCL] Address space for default class members

rjmccall added inline comments. Comment at: lib/Sema/SemaInit.cpp:4539 + if (InitCategory.isPRValue() || InitCategory.isXValue()) +T1Quals.removeAddressSpace(); + ebevhan wrote: > rjmccall wrote: > > rjmccall wrote: > > > I can understand why a pr-value woul

[PATCH] D55662: [Sema][ObjC] Do not warn about repeated uses of weak variables when the variables are accessed in an unevaluated context.

rjmccall added a comment. If it's at all reasonable to just avoid doing the work multiple times, let's do that. It should also avoid duplicate diagnostics if e.g. an overload is resolved to an unavailable function. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D55948: Modify DeclaratorChuck::getFunction to use DeclSpec for qualifiers

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

[PATCH] D56367: [AST] Pack CXXDependentScopeMemberExpr

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

[PATCH] D55662: [Sema][ObjC] Do not warn about repeated uses of weak variables when the variables are accessed in an unevaluated context.

rjmccall added inline comments. Comment at: lib/Sema/SemaLambda.cpp:793 + else +Args = Init; + Please maintain the original order here, even though I suspect it doesn't matter: if this is direct-initialization, use the arguments, otherwise use either `Dedu

[PATCH] D55662: [Sema][ObjC] Do not warn about repeated uses of weak variables when the variables are accessed in an unevaluated context.

rjmccall added a comment. Oh, and please update the commit message to primarily talk about the changes to placeholder checking. You can discuss the impact on the repeated-use-of-weak warning in a follow-up paragraph. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org

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

rjmccall added a comment. Sema won't necessarily have resolved a template decl when parsing a template argument list, so trying to propagate that decl down to indicate that we're resolving a template argument is not a good approach. I was going to suggest recording that we're within a template

[PATCH] D55781: Make CC mangling conditional on the ABI version

rjmccall added a comment. Okay. In my opinion, then, we should just do this unconditionally. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55781/new/ https://reviews.llvm.org/D55781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

[PATCH] D54188: [Sema] Mark target of __attribute__((alias("target"))) used for C

rjmccall added a comment. I really dislike this particular idiom. This is probably the most reasonable short-term solution to this problem. I do wonder if we should just shift to making Sema track a symbol-to-GD map, though, and maybe make Sema entirely responsible for pushing entities to IRGe

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

rjmccall added a comment. But why? Why do you want to limit this to just template arguments instead of all sorts of similar contexts? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56411/new/ https://reviews.llvm.org/D56411 ___ cfe-commits

[PATCH] D54188: [Sema] Mark target of __attribute__((alias("target"))) used for C

rjmccall added inline comments. Comment at: test/Sema/alias-unused.c:3 +// expected-no-diagnostics +int f() { return 42; } +int g() __attribute__((alias("f"))); nickdesaulniers wrote: > rjmccall wrote: > > Is this meant to be `static`? > It doesn't make a differe

[PATCH] D56066: [OpenCL] Address space for default class members

rjmccall added inline comments. Comment at: lib/Sema/SemaInit.cpp:4539 + if (InitCategory.isPRValue() || InitCategory.isXValue()) +T1Quals.removeAddressSpace(); + Anastasia wrote: > rjmccall wrote: > > ebevhan wrote: > > > rjmccall wrote: > > > > rjmccall wr

[PATCH] D54188: [Sema] Mark target of __attribute__((alias("target"))) used for C

rjmccall added a comment. Don't worry, that's a familiar mistake. :) LGTM. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54188/new/ https://reviews.llvm.org/D54188 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D55844: [Fixed Point Arithmetic] Fixed Point Subtraction

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3451 + case BO_Comma: +llvm_unreachable("Found unimplemented fixed point binary operation"); } Please create a separate case for the non-arithmetic operators (pointer-to-membe

[PATCH] D55662: [Sema] If CheckPlaceholderExpr rewrites a placeholder expression when the type of an auto variable is being deduced, use the rewritten expression when performing initialization of the

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Sounds good. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55662/new/ https://reviews.llvm.org/D55662 ___ cf

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

rjmccall added a comment. This patch still doesn't make any sense. You don't need to do any special validation when passing a function as a template argument. When Sema instantiates the template definition, it'll rebuild the expressions that refer to the template parameter, which will trigger

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. If I was only concerned about `size_t`, your current solution would be fine. My concern is that you really need to match *all* of the associated CPU target's ABI choices, so your target really ought to be forwarding everything to that target by default and only select

[PATCH] D56225: [HIP] Use nul instead of /dev/null when running on windows

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: lib/Driver/ToolChains/HIP.cpp:210 std::string BundlerTargetArg = "-targets=host-x86_64-unknown-linux"; - std::string BundlerInputArg = "-inputs=/dev/n

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. No, I understand that things like the function-call ABI should be different from the associated host ABI, but things like the size of `long` and the bit-field layout algorithm presumably shouldn't be, and that's the sort of thing that's configured by `TargetInfo`. CH

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. In D56318#1353116 , @yaxunl wrote: > In D56318#1353106 , @rjmccall wrote: > > > No, I understand that things like the function-call ABI should be different > > from the associated host ABI

[PATCH] D56066: [OpenCL] Address space for default class members

rjmccall added inline comments. Comment at: lib/AST/Expr.cpp:1618 assert(getValueKindForType(Ty) == Expr::getValueKindForType(SETy)); -if (!isGLValue()) +if (!isGLValue() && !getSubExpr()->isXValue()) { Ty = Ty->getPointeeType(); What's this a

[PATCH] D66121: Debug Info: Nest Objective-C property function decls inside their container.

rjmccall added a comment. Right, that sounds reasonable. You should be making them link up exactly like a normal method implementation, so if SemaObjC doesn't consider normal method implementations to be redeclarations, then I guess these aren't either. And if we want to change that in the fu

[PATCH] D67429: Improve code generation for thread_local variables:

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

[PATCH] D67399: [ARM] Follow AACPS standard for volatile bitfields

rjmccall added a comment. The exact sequence of volatile accesses is observable behavior, and it's the ABI's right to define correct behavior for compliant implementations, so we do need to do this. Diogo, IRGen breaks up bit-field storage units in CGRecordLowering::accumulateBitFields. I'm s

[PATCH] D67399: [ARM] Follow AACPS standard for volatile bitfields

rjmccall added a comment. I have to say that I disagree. The ABI certainly doesn't get to control the language and define what constitutes a volatile access, but when the language has decided that a volatile access is actually performed, I think ABIs absolutely ought to define how they should

[PATCH] D67399: [ARM] Follow AACPS standard for volatile bitfields

rjmccall added a comment. In D67399#1669568 , @jfb wrote: > In D67399#1669038 , @dnsampaio wrote: > > > Indeed our main concern is regarding the access widths of loads. As > > mentioned by @rjmccall, most volatile

[PATCH] D66121: Debug Info: Nest Objective-C property function decls inside their container.

rjmccall added a comment. In D66121#1673233 , @aprantl wrote: > I'm afraid I'm going to give up on fixing the AST and return to my > debuginfo-only patch. > > While I was correct in figuring out that ObjCMethodDecl implementations are > not linked up as

[PATCH] D67774: [Mangle] Check ExternalASTSource before adding prefix to asm label names

rjmccall added inline comments. Comment at: clang/lib/AST/Mangle.cpp:137 +(!Source || Source->UseGlobalPrefixInAsmLabelMangle())) Out << '\01'; // LLVM IR Marker for __asm("foo") This is one of those bugs where looking at the code really reveals

[PATCH] D67837: [CUDA][HIP] Fix assertion in Sema::markKnownEmitted with -fopenmp

rjmccall added inline comments. Comment at: lib/Sema/Sema.cpp:1511 +if (Loc != S.DeviceCallGraph.end()) + S.DeviceCallGraph.erase(Loc); return; There's an overload of `DenseMap::erase` that just takes a key value, so this whole thing can be `S.Devi

[PATCH] D67774: [Mangle] Add flag to asm labels to disable global prefixing

rjmccall added inline comments. Comment at: clang/include/clang/Basic/Attr.td:725 let Spellings = [Keyword<"asm">, Keyword<"__asm__">]; - let Args = [StringArgument<"Label">]; + let Args = [StringArgument<"Label">, BoolArgument<"LiteralLabel">]; let SemaHandler = 0; -

[PATCH] D67837: [CUDA][HIP] Fix assertion in Sema::markKnownEmitted with -fopenmp

rjmccall added inline comments. Comment at: lib/Sema/Sema.cpp:1511 +if (Loc != S.DeviceCallGraph.end()) + S.DeviceCallGraph.erase(Loc); return; yaxunl wrote: > rjmccall wrote: > > There's an overload of `DenseMap::erase` that just takes a key value,

[PATCH] D67837: [CUDA][HIP] Fix assertion in Sema::markKnownEmitted with -fopenmp

rjmccall added a comment. Okay. And it's okay to fall down to the code below when functions are used in both ways this way? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67837/new/ https://reviews.llvm.org/D67837 ___ cfe-commits mailing l

[PATCH] D67774: [Mangle] Add flag to asm labels to disable global prefixing

rjmccall added inline comments. Comment at: clang/include/clang/Basic/Attr.td:734 +// TODO: Make it possible to specify this in source. +BoolArgument<"LiteralLabel"> + ]; `LiteralLabel` is an unfortunate name for this property because `getLiteralLabel`

[PATCH] D67837: [CUDA][HIP] Fix assertion in Sema::markKnownEmitted with -fopenmp

rjmccall added a comment. If that's tractable, then that does seem like the best solution. You can commit this if you need a shorter-term fix. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67837/new/ https://reviews.llvm.org/D67837 ___ cf

[PATCH] D67774: [Mangle] Add flag to asm labels to disable global prefixing

rjmccall added inline comments. Comment at: clang/lib/AST/Mangle.cpp:130 + return; +} + vsk wrote: > rjmccall wrote: > > This is actually backwards, right? A literal label is one that doesn't get > > the global prefix and therefore potentially needs th

[PATCH] D67774: [Mangle] Add flag to asm labels to disable '\01' prefixing

rjmccall added a comment. Thanks. One last and minor request: please mention in the comment on IsLiteralLabel that non-literal labels are used by some external AST sources like LLDB. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67774/new/ https://reviews.llvm.org/D67774 __

[PATCH] D67774: [Mangle] Add flag to asm labels to disable '\01' prefixing

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

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:5998 +QualType Type = Var->getType(); +if (Type->isSamplerT() || Type->isVoidType()) + return; Anastasia wrote: > I don't seem to need a check for dependent or auto types because

[PATCH] D67837: [CUDA][HIP] Fix host/device check with -fopenmp

rjmccall added a comment. That looks great. I'm probably not the best person to validate that the OMP / CUDA logic is still right, but it seems like the right technical design. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67837/new/ https://reviews.llvm.org/D67837

[PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

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/D67983/new/ https://reviews.llvm.org/D67983 __

[PATCH] D67982: [ObjC] Add some additional test cases around pointer conversions.

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/D67982/new/ https://reviews.llvm.org/D67982 __

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:6721 + if (getLangOpts().OpenCL) { + Anastasia wrote: > rjmccall wrote: > > Since you're moving this code anyway, can this be split into its own > > function? I'm not sure if it's actual

[PATCH] D68108: Redeclare Objective-C property accessors inside the ObjCImplDecl in which they are synthesized.

rjmccall added inline comments. Comment at: clang/include/clang/AST/DeclObjC.h:2787 + /// interface as a decl context. + ObjCMethodDecl *SetterMethodDecl = nullptr; + Are these comments right? The DC is the implementation's interface? Regardless, I think thes

[PATCH] D68108: Redeclare Objective-C property accessors inside the ObjCImplDecl in which they are synthesized.

rjmccall added a comment. Also, I think it would be good if there was a more explicit method for asking whether a method is a synthesized definition. Basically, there's a tri-state: there are pure declarations, user-provided definitions, and synthetic definitions. IRGen should emit a function

[PATCH] D44984: [HIP] Add hip file type and codegen for kernel launching

rjmccall added inline comments. Comment at: lib/CodeGen/CGCUDANV.cpp:98 +std::string CGNVCUDARuntime::addPrefixToName(CodeGenModule &CGM, + std::string FuncName) const { + if (CGM.getLangOpts().HIP) Can you take these

[PATCH] D45451: [ItaniumMangle] Undeduced auto type doesn't belong in the substitution table

rjmccall added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:2342 + if (isa(Ty)) +return false; return true; I agree with your analysis that this shouldn't be a substitution candidate. However, I think this probably needs an ABI-compatibi

[PATCH] D45212: [HIP] Let CUDA toolchain support HIP language mode and amdgpu

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I'd still prefer if someone with more driver-design expertise weighed in, but we might not have any specialists there. LGTM, although you might consider changing your tests a bit: FileChec

[PATCH] D45578: Add a command line option 'fregister_dtor_with_atexit' to register destructor functions annotated with __attribute__((destructor)) using __cxa_atexit or atexit.

rjmccall added inline comments. Comment at: include/clang/Driver/Options.td:1613 +def fregister_dtor_with_atexit : Flag<["-"], "fregister-dtor-with-atexit">, Group, Flags<[CC1Option]>, + HelpText<"Use atexit or __cxa_atexit to register destructors">; def fuse_init_array : Flag

[PATCH] D44984: [HIP] Add hip file type and codegen for kernel launching

rjmccall added inline comments. Comment at: lib/Frontend/CompilerInvocation.cpp:2109 + Opts.HIP = true; + } + yaxunl wrote: > rjmccall wrote: > > Why is this done here? We infer the language mode from the input kind > > somewhere else. > It is usually don

[PATCH] D45489: [HIP] Add input type for HIP

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

[PATCH] D45441: [HIP] Add predefined macros __HIPCC__ and __HIP_DEVICE_COMPILE__

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. If so, LGTM. Comment at: lib/Frontend/InitPreprocessor.cpp:473 + Builder.defineMacro("__HIP_DEVICE_COMPILE__"); + } } I assume these names are def

[PATCH] D45578: Add a command line option `fregister_global_dtors_with_atexit` to register destructor functions annotated with __attribute__((destructor)) using __cxa_atexit or atexit.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thank you. Minor request and then LGTM. Comment at: lib/CodeGen/ItaniumCXXABI.cpp:2202 +// __attribute__((destructor)) in a constructor function. +addr = llvm::C

[PATCH] D45310: Warn about memcpy'ing non-trivial C structs

rjmccall added inline comments. Comment at: lib/Sema/SemaChecking.cpp:7651 + << PointeeTy << 1); + SearchNonTrivialToCopyField::diag(PointeeTy, Dest, *this); + } else { Indentation seems messed up in these two clause

[PATCH] D45310: Warn about memcpy'ing non-trivial C structs

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

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

rjmccall added a comment. The goal of having a unified option is that you can uniformly suppress warnings that don't always make sense in unit tests. It's future-proofing against the addition of other warnings (probably C++ warnings) that might not make sense for unit tests, like extending the

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

rjmccall added a comment. I'm sorry, that warning *is* in self-assign, although I'm not sure it should be. Repository: rC Clang https://reviews.llvm.org/D45685 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

rjmccall added a comment. Let me try to summarize where I think we are. 1. I think it’s now generally agreed that this is a useful warning — certainly for field assignments, but we also have (at least?) one example with a local variable. 2. It’s also generally agreed that this warning has a pro

[PATCH] D45223: [CUDA] Fix overloading resolution failure due to kernel calling convention

rjmccall added a comment. Yes, I'm sorry, I think you're right. I had misunderstood the language problem when I suggested going down this road. https://reviews.llvm.org/D45223 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

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

rjmccall added a comment. These functions must predate the addition of CreateLifetimeStart and CreateLifetimeEnd methods on IRBuilder; we should just be calling those. https://reviews.llvm.org/D45900 ___ cfe-commits mailing list cfe-commits@lists.l

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

rjmccall added a reviewer: doug.gregor. rjmccall added inline comments. Comment at: lib/Sema/SemaInit.cpp:7074 + if (RD->hasDefinition() && RD->hasNonTrivialDestructor()) +S.MarkFunctionReferenced(E->getExprLoc(), S.LookupDestructor(RD)); + I

[PATCH] D44435: CUDA ctor/dtor Module-Unique Symbol Name

rjmccall added a comment. This does not address my review. My review is suggesting that we avoid this issue completely by fixing IRGen to use an external linkage for internal declarations in your emission mode. That would allow you to just emit the module ctors as truly internal in the first

[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

rjmccall added inline comments. Herald added a reviewer: javed.absar. Comment at: lib/CodeGen/CodeGenFunction.h:847 +CurrentCleanupStackDepth = C; + } + You don't need (or want) these accessors, I think; this is just private state of the CGF object, and nob

[PATCH] D45451: [ItaniumMangle] Undeduced auto type doesn't belong in the substitution table

rjmccall added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:2342 + if (isa(Ty)) +return false; return true; rjmccall wrote: > I agree with your analysis that this shouldn't be a substitution candidate. > However, I think this probably ne

[PATCH] D45451: [ItaniumMangle] Undeduced auto type doesn't belong in the substitution table

rjmccall added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:2342 + if (isa(Ty)) +return false; return true; rjmccall wrote: > rjmccall wrote: > > I agree with your analysis that this shouldn't be a substitution candidate. > > However, I

[PATCH] D45451: [ItaniumMangle] Undeduced auto type doesn't belong in the substitution table

rjmccall added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:2342 + if (isa(Ty)) +return false; return true; erik.pilkington wrote: > rjmccall wrote: > > rjmccall wrote: > > > rjmccall wrote: > > > > I agree with your analysis that this sho

[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3069 + if (hasAggregateEvaluationKind(type) && + getContext().isParamDestroyedInCallee(type)) { +EHScopeStack::stable_iterator cleanup = I wonder if this is something we should be taking

[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3069 + if (hasAggregateEvaluationKind(type) && + getContext().isParamDestroyedInCallee(type)) { +EHScopeStack::stable_iterator cleanup = ahatanak wrote: > rjmccall wrote: > > I wonder if

[PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

rjmccall added a comment. In https://reviews.llvm.org/D45766#1076090, @dblaikie wrote: > FWIW I don't fundamentalyl object to also having something like -wtest. > Probably needs a better name though (unfortunately the double-negative gets > confusing... - like you want to describe the set of di

[PATCH] D45766: [Sema] Add -Wno-self-assign-overloaded

rjmccall added a comment. In https://reviews.llvm.org/D45766#1076176, @dblaikie wrote: > Is there anything else in the "-w" namespace other than the literal "-w" so > far? No. This would be novel. > I mean, I could imagine it might make more sense to default these warnings > off & users can

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:1002 + return EmitFinalDestCopy( + E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType())); +} Is there something in Sema actually validating that the comparison types is tri

<    7   8   9   10   11   12   13   14   15   16   >