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

rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:1138 + +return MetadataAsValue::get(Context, RoundingMDS); + } kpn wrote: > rjmccall wrote: > > Huh? You build an `MDNode` that wraps an `MDString` and then immediately > > extract the

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

rjmccall added a comment. Minor requests, then LGTM. Comment at: lib/CodeGen/CGExprConstant.cpp:73 +/// Incremental builder for an llvm::Constant* holding a structure constant. +class ConstantBuilder : private ConstantBuilderUtils { + llvm::SmallVector Elems;

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

rjmccall added a comment. In D59744#1548540 , @mtklein wrote: > Hey folks, I'm the Skia point of contact on this, and "luckily" the person > who wrote all the code that got us into this mess. Let me cross post a > couple questions I've had from the Chro

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

rjmccall added inline comments. Comment at: include/llvm/IR/IntrinsicInst.h:235 + ebStrict ///< This corresponds to "fpexcept.strict". }; Is it okay that `ebUnspecified` and `ebInvalid` overlap here? Comment at: include/ll

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

rjmccall added a comment. In D59744#1548919 , @efriedma wrote: > > Now, we could theoretically use a different ABI rule for vectors defined > > with Clang-specific extensions, but that seems like it would cause quite a > > few problems of its own. > > I

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

rjmccall added inline comments. Comment at: include/llvm/IR/IntrinsicInst.h:235 + ebStrict ///< This corresponds to "fpexcept.strict". }; kpn wrote: > rjmccall wrote: > > Is it okay that `ebUnspecified` and `ebInvalid` overlap here? > I can

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

rjmccall added a comment. Thank you. So it sounds like this patch needs to be reverted, and the correct version of it will have to insert these intrinsic calls in four places: - before translating vector arguments to MMX type before calls that pass `__m64` arguments, - after translating MMX pa

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

rjmccall added a comment. In D59744#1549229 , @efriedma wrote: > If we're going to insert emms instructions automatically, it doesn't really > make sense to do it in the frontend; the backend could figure out the most > efficient placement itself. (See

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

rjmccall added a comment. Can this attribute not be applied to a base class, or to a type? I think every time I've seen someone get bitten by the unique-address rule, it was because they had a base class that deleted some constructors (or something like that) and which was a base of a million

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

rjmccall added inline comments. Comment at: lib/AST/Decl.cpp:3937 + // -- [has] virtual member functions or virtual base classes, or + // -- has subobjects of nonzero size or bit-fields of nonzero length + if (const auto *CXXRD = dyn_cast(RD)) { rsmith

[PATCH] D63277: [CUDA][HIP] Don't set "comdat" attribute for CUDA device stub functions.

rjmccall added a comment. This optimization is disabled for functions not in COMDAT sections? Is that documented somewhere? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63277/new/ https://reviews.llvm.org/D63277 ___ cfe-commits mailing l

[PATCH] D63277: [CUDA][HIP] Don't set "comdat" attribute for CUDA device stub functions.

rjmccall accepted this revision. rjmccall added a comment. Alright, thanks. I agree that per the documentation this should be sufficient. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63277/new/ https://reviews.llvm.org/D63277 ___ cfe-comm

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

rjmccall accepted this revision. rjmccall added inline comments. Comment at: include/clang/Basic/Attr.td:326 + // const llvm::Triple &T = Target.getTriple(); + code CustomCode = [{}]; } Thanks! Comment at: lib/CodeGen/CGExprAgg.cpp:1850 +A

[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

rjmccall added inline comments. Comment at: cfe/trunk/lib/Sema/SemaExprCXX.cpp:4229 +LangAS AddrSpaceR = +RHSType->getAs()->getPointeeType().getAddressSpace(); +CastKind Kind = Anastasia wrote: > rjmccall wrote: > > Anastasia wrote: > > > rjmccall

[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] D64400: [OpenCL][PR42390] Deduce addr space for templ specialization

rjmccall added a comment. There are some code paths that I think are common between the parser and template instantiation, like `BuildPointerType` and `BuildReferenceType`, but if you want to do context-sensitive inference that might not be good enough. CHANGES SINCE LAST ACTION https://revi

[PATCH] D63975: Warn when ScopeDepthOrObjCQuals overflows

rjmccall added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:6587 +return; + } + Mordante wrote: > rjmccall wrote: > > Comment indentation. > > > > Should we do this when starting to parse a function prototype instead of > > when parsi

[PATCH] D63975: Warn when ScopeDepthOrObjCQuals overflows

rjmccall added a comment. Thanks. It's good to have a lambda test, but that one isn't actually testing the lambda path — the place the diagnostic will trigger is just the normal function-prototype path, just originally within a lambda. You can do something like this: template int foo(T &&

[PATCH] D63975: Warn when ScopeDepthOrObjCQuals overflows

rjmccall added a comment. (Blocks don't actually allow default arguments, but apparently we still parse them, so we should test that path.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63975/new/ https://reviews.llvm.org/D63975 ___ cfe-co

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

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

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

rjmccall added a comment. If you're interested in working on this, great. I actually think there's zero reason to emit a non-null argument here on any target unless we're going to use the destructor as the function pointer — but we can do that on every Itanium target, so we should. So where w

[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

rjmccall added a comment. A qualifier "outside" the block-pointer type is telling you what address space the object which holds the block pointer is in, which is a completely different thing. Instead of `__global int (^block4)(void) = ^{ return 0; };`, you need to write `int (^__global block4)

[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

rjmccall added a comment. In D64083#1583109 , @rjmccall wrote: > Okay, so it sounds like *from the language perspective* all block pointers > are actually pointers into `__generic`, and the thing with literals is just > an implementation detail that's be

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

rjmccall added a comment. Oh, yes, it definitely can't be done to class types. I suppose we should just forget about it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62584/new/ https://reviews.llvm.org/D62584 ___ cfe-commits mailing list

[PATCH] D64400: [OpenCL][PR42390] Deduce addr space for templ specialization

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

[PATCH] D62960: Add SVE opaque built-in types

rjmccall added a comment. Seems reasonable to me. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62960/new/ https://reviews.llvm.org/D62960 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.or

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGClass.cpp:2016 CGF.EmitCXXDestructorCall(dtor, Dtor_Complete, /*for vbase*/ false, -/*Delegating=*/false, addr); +/*Delegating=*/false, addr, type); } -

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprAgg.cpp:1627 +llvm::Constant::getNullValue(CGF.Int8PtrTy), +CharUnits::One()); // placeholder + Please declare a lambda to add a cleanup which implicitly creates this dominator a

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

rjmccall added inline comments. Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:2 +// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s +// CHECK no crash + oydale wrote: > lebedev.ri wrote: > > Either add miss

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

rjmccall added a comment. Oh, I see you've already answered that. I think it's fine to just leave this testing debug output if generated optimized output doesn't affect it; the bulk of our regression testing is with assertions-enabled compilers anyway. Repository: rG LLVM Github Monorepo C

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

rjmccall added a comment. Thanks, just a few minor comment requests now. Comment at: include/clang/AST/DeclBase.h:1453 +/// copy. +uint64_t HasNonTrivialToPrimitiveCopyCUnion : 1; + Please include in these comments that these imply the associated basic

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

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

[PATCH] D63975: Warn when ScopeDepthOrObjCQuals overflows

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

[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

rjmccall added a comment. Sounds good. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64083/new/ https://reviews.llvm.org/D64083 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/ma

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

rjmccall added a comment. In D62584#1585091 , @Anastasia wrote: > In D62584#1583340 , @rjmccall wrote: > > > Oh, yes, it definitely can't be done to class types. I suppose we should > > just forget about it. > > >

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

rjmccall added a comment. Minor comment then LGTM Comment at: lib/Sema/SemaType.cpp:7418 + // Expect for pointer or reference types because the addr space in + // template argument can only belong to a pointee. + (T->isDependentType() && !T->isPointerType() && !T

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGClass.cpp:2016 CGF.EmitCXXDestructorCall(dtor, Dtor_Complete, /*for vbase*/ false, -/*Delegating=*/false, addr); +/*Delegating=*/false, addr, type); } -

[PATCH] D64804: [OpenCL][Sema] Minor refactoring and constraint checking

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

[PATCH] D64780: Disallow most calling convention attributes on PS4.

rjmccall added a comment. I'm fine with the idea of targets making unsupported CCs hard errors. Comment at: test/Sema/no_callconv.cpp:44 +void __attribute__((sysv_abi)) funcI() {} +void __attribute__((cdecl)) funcC() {} Please include a newline at the end of th

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

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

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

rjmccall added a comment. LGTM with one minor request. Comment at: clang/lib/AST/DeclCXX.cpp:2267 QualType ClassTy = C.getTypeDeclType(Decl); - ClassTy = C.getQualifiedType(ClassTy, FPT->getMethodQuals()); - return C.getPointerType(ClassTy); + return C.getQualifiedType(Cl

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

rjmccall added a comment. Yes, that's the right fix, although you might also consider adding a `getObjectType()` to `CXXMemberCallExpr`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64569/new/ https://reviews.llvm.org/D64569 __

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

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

[PATCH] D65286: [OpenCL] Allow OpenCL C style vector initialization in C++

rjmccall added a comment. Wait, plain C++? Do we currently allow this syntax outside of OpenCL? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65286/new/ https://reviews.llvm.org/D65286 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

rjmccall added a comment. These were unavailable in system headers before because otherwise we would've had to make them invalid. Since these unions are no longer otherwise invalid, there shouldn't be a problem with allowing them in system headers, and in fact making the semantics vary that wa

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

rjmccall added a comment. In D65256#1601415 , @jordan_rose wrote: > In D65256#1601410 , @rjmccall wrote: > > > These were unavailable in system headers before because otherwise we > > would've had to make them inva

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

rjmccall added a comment. In D65256#1601510 , @jordan_rose wrote: > In D65256#1601509 , @rjmccall wrote: > > > Sorry, am I missing something? Such a union would've been either > > ill-formed or unavailable in ARC

[PATCH] D65286: [OpenCL] Allow OpenCL C style vector initialization in C++

rjmccall added a comment. Okay. It seems reasonable to me to allow OpenCL vector initialization syntax in OpenCL C++, since we're treating OpenCL C++ as an ObjC++-like merge of the language extensions, but we shouldn't go further than that and start changing behavior in the non-OpenCL language

[PATCH] D64932: [Parser] Emit descriptive diagnostic for misplaced pragma

rjmccall added inline comments. Comment at: clang/lib/Basic/TokenKinds.cpp:55 +return std::strncmp(#X, "pragma_", sizeof("pragma_") - 1) == 0; +#include "clang/Basic/TokenKinds.def" + default: The right way to do this is to make a `PRAGMA_ANNOTATION` macro i

[PATCH] D64811: Warn when NumParams overflows

rjmccall added inline comments. Comment at: clang/include/clang/AST/Type.h:1523 + enum { NumParamsBits = 16 }; + This should probably go in `FunctionTypeBitfields`. Comment at: clang/lib/Parse/ParseDecl.cpp:6673 + // Avoid exceeding the max

[PATCH] D64811: Warn when NumParams overflows

rjmccall added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:6673 + // Avoid exceeding the maximum function parameters + // See https://bugs.llvm.org/show_bug.cgi?id=19607 + if (ParamInfo.size() > Type::getMaxNumParams()) { rjmccall wrote: > We do

[PATCH] D65286: [OpenCL] Allow OpenCL C style vector initialization in C++

rjmccall added a comment. In D65286#1606441 , @Anastasia wrote: > In D65286#1606071 , @mantognini > wrote: > > > In `vector_literals_nested.cl`, we have tests for (global) constants. Do > > you think it would be w

[PATCH] D65406: [Parser] Change parameter type from int to enum

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

[PATCH] D64932: [Parser] Emit descriptive diagnostic for misplaced pragma

rjmccall added inline comments. Comment at: clang/lib/Basic/TokenKinds.cpp:15 #include "llvm/Support/ErrorHandling.h" +#include using namespace clang; This is no longer necessary, right? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

[PATCH] D65405: [Parser] Use special definition for pragma annotations

rjmccall added a comment. LGTM, although I don't think it would be ridiculous to make this an inline function definition like `isAnnotation`. (I like your generated implementation much better than the sequential `if`s used there, though.) Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D64811: Warn when NumParams overflows

rjmccall added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:6673 + // Avoid exceeding the maximum function parameters + // See https://bugs.llvm.org/show_bug.cgi?id=19607 + if (ParamInfo.size() > Type::getMaxNumParams()) { Mordante wrote: > rjmcc

[PATCH] D64932: [Parser] Emit descriptive diagnostic for misplaced pragma

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

[PATCH] D64464: [CodeGen] Emit destructor calls for non-trivial C structs

rjmccall added inline comments. Herald added a subscriber: rnkovacs. Comment at: include/clang/AST/ExprCXX.h:3220 + /// It's useful to remember the set of blocks and compound literals; we could + /// also remember the set of temporaries, but there's currently no need. + using

[PATCH] D65670: Use switch instead of series of comparisons

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

[PATCH] D65597: WIP: Builtins: Start adding half versions of math builtins

rjmccall added a comment. In N2405 (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2405.pdf), these are called `truncf16`, `cosf16`, etc., which I think is appropriate. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65597/new/ https://reviews.llvm.org/D65597 ___

[PATCH] D65597: WIP: Builtins: Start adding half versions of math builtins

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

[PATCH] D65753: Builtins: Add some v2f16 variants

rjmccall added a comment. Hmm. I think there are two reasonable concerns here, both arising from the fact that these names occupy (the builtin namespace parallel to) a namespace that the committee might want to occupy in the future: - Is a `v2fN` suffix sufficiently likely to avoid interfering

[PATCH] D65753: Builtins: Add some v2f16 variants

rjmccall added a comment. You might want to look into ISO/IEC TS 19570 - 2018 to see what they're thinking. If it looks like there's a potential conflict, then maybe these should have an OpenCL-specific prefix on them. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65753/new/ https://

[PATCH] D65753: Builtins: Add some v2f16 variants

rjmccall added a comment. Hmm, sorry, that's a C++ spec, and it looks like the (abandoned) C attempt at data parallelism (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2081.htm) doesn't introduce vector types. Talking about it internally, I think there are two reasonable approaches: - Add a

[PATCH] D54565: Introduce `-Wctad` as a subgroup of `-Wc++14-compat`

rjmccall added a comment. I doubt "CTAD" is going to survive as a term that people recognize and remember, and I don't think `-Wclass-template-argument-deduction` is outrageously wordy compared to some of our other diagnostic groups. With that change, this would be fine with me. Repository:

[PATCH] D54565: Introduce `-Wctad` as a subgroup of `-Wc++14-compat`

rjmccall added a comment. Oh, I see this review might be dead in the water; I'm not sure why I was just added as a reviewer to it (by a non-author, no less). I personally have no problem with pulling out specific features as sub-groups of the compatibility warning. I agree with Richard, though

[PATCH] D62960: Add SVE opaque built-in types

rjmccall added inline comments. Comment at: lib/AST/ASTContext.cpp:1974 + break; +#include "clang/Basic/AArch64SVEACLETypes.def" } Why do SVE predicates have 16-bit alignment? Should this be 128-bit (16-*byte*)? I guess these alignments are reasonabl

[PATCH] D62960: Add SVE opaque built-in types

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: lib/AST/ASTContext.cpp:1974 + break; +#include "clang/Basic/AArch64SVEACLETypes.def" } rsandifo-arm wrote: > rjmccall wrote:

[PATCH] D65994: Extended FPOptions with new attributes

rjmccall added a comment. Since this setting changes IR output, you should write a test that emits IR for source code and tests that you get the right IR output. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65994/new/ https://reviews.llvm.org/D65

[PATCH] D47419: [SemaDeclCXX] Allow inheriting constructor declaration that specify a cv-qualified type

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: lib/Sema/SemaDeclCXX.cpp:9690 + CanQualType CanonicalDesiredBase = DesiredBase->getCanonicalTypeUnqualified() +.getUnqualifiedType(); for (auto &B

[PATCH] D65665: Support for attributes on @class declarations

rjmccall added a comment. Have you looked through the attributes that can be written on `@interface`s and verified that they're all sensible to write on a `@class`? It's not hard to imagine that *some* of them should be diagnosed when added to a non-definition. Repository: rC Clang CHANGES

[PATCH] D65994: Extended FPOptions with new attributes

rjmccall added a comment. Oh, sorry, I confused several patches, then. I don't actually think you need to break down the patches this finely; it would be fine to take all three steps to implement the feature in one pass. It's only important to break down the patches into more incremental compo

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

rjmccall added a comment. Isn't the general rule for template argument deduction (which this devolves to) just to ignore top-level qualifiers? And then you can substitute in the substituted type and end up with a properly qualified type for the parameter / variable, and you can add extra quali

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.h:406 + CGBuilderTy &Builder, + const ObjCContainerDecl *CD = nullptr); Why does this have to be an extra parameter? The method's DC shoul

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

rjmccall added a comment. I see. Is the deduction rule recursive or something, where a pointer to pointer is inferred to point to the same address space as the pointee? I still don't understand why pointers and references are handled differently here, instead of having the rule be "don't infe

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.h:406 + CGBuilderTy &Builder, + const ObjCContainerDecl *CD = nullptr); aprantl wrote: > rjmccall wrote: > > Why does this have to be an ex

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.h:406 + CGBuilderTy &Builder, + const ObjCContainerDecl *CD = nullptr); aprantl wrote: > rjmccall wrote: > > aprantl wrote: > > > rjmccall

[PATCH] D27165: Add format_dynamic_key_arg attribute to improve "-Wformat" warnings for functions that load the formatting string dynamically based on a key value

rjmccall added a comment. I agree with just special-casing this for now. Comment at: clang/lib/Sema/SemaChecking.cpp:6800 +MD->getSelector() == +S.NSAPIObj->getLocalizedStringForKeySelector()) { + IgnoreStringsWithoutSpecifiers = true; -

[PATCH] D66094: [CodeGen] Emit destructor calls for non-trivial C structs returned by function calls and loaded from volatile objects

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.h:372 + REQUIRES_DESTRUCTION = 0x4, }; Does `llvm::Value*` actually guarantee three bits? Presumably on 64-bit platforms, but we do support 32-bit hosts. Fortunately, I don't actu

[PATCH] D27165: Add format_dynamic_key_arg attribute to improve "-Wformat" warnings for functions that load the formatting string dynamically based on a key value

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. One minor request (which will be dead code in your patch), but otherwise LGTM. Comment at: clang/include/clang/Basic/IdentifierTable.h:754 + /// If this selector is the

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

rjmccall added a comment. In D63451#1552609 , @rsmith wrote: > In D63451#1549563 , @rjmccall wrote: > > > Can this attribute not be applied to a base class, or to a type? > > > The standard attribute forbids that ri

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

rjmccall added inline comments. Comment at: include/llvm/IR/IntrinsicInst.h:235 + ebStrict ///< This corresponds to "fpexcept.strict". }; kpn wrote: > rjmccall wrote: > > kpn wrote: > > > rjmccall wrote: > > > > Is it okay that `ebUnspecifie

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

rjmccall added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:633 + "capture a variable of type %1}3 " + "since it %select{contains a union that |}2is non-trivial to " + "%select{default-initialize|destruct|copy}0">; Should this be "`%

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11085 +if (isa(I)) + continue; +if (auto E = dyn_cast(I)) ahatanak wrote: > rjmccall wrote: > > Why is this okay? Don't we need to check default-initialization for these? > I didn't c

[PATCH] D63856: [ObjC] Add a -Wtautological-compare warning for BOOL

rjmccall added a comment. This only applies to relational operators, right? I'm a little uncomfortable with calling this "tautological" since it's not like it's *undefined behavior* to have `(BOOL) 2`, it's just *unwise*. But as long as we aren't warning about reasonable idioms that are inten

[PATCH] D63856: [ObjC] Add a -Wtautological-compare warning for BOOL

rjmccall added a comment. In D63856#1560213 , @erik.pilkington wrote: > In D63856#1560180 , @rjmccall wrote: > > > This only applies to relational operators, right? I'm a little > > uncomfortable with calling thi

[PATCH] D63856: [ObjC] Add a -Wtautological-compare warning for BOOL

rjmccall added a comment. In D63856#1561132 , @erik.pilkington wrote: > In D63856#1561112 , @rjmccall wrote: > > > In D63856#1560213 , > > @erik.pilkington wrote: > > > > >

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

rjmccall added a comment. I'm not sure we've ever written down what the semantics of the block capture are actually supposed to be in this situation. I guess capturing a reference is the right thing to do? Is that what we generally do if this is a POD type? Repository: rC Clang CHANGES SI

[PATCH] D53295: Mark store and load of block invoke function as invariant.group

rjmccall added a comment. Great, thank you. Yaxun, are you planning to pick this back up? I know it's been a long time. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53295/new/ https://reviews.llvm.org/D53295 ___ cfe-commits mailing list

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

rjmccall added a comment. Are the `CaseStmt`s being dropped in C++ because the expression overflows? I agree that that's bad AST behavior; we should strive to generate AST whenever we can, even if it's not valid. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63139/new/ https://review

[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] D63260: [Attr] Support _attribute__ ((fallthrough))

rjmccall added inline comments. Comment at: lib/Sema/AnalysisBasedWarnings.cpp:1279 continue; - if (S.getLangOpts().CPlusPlus11) { + if (S.getLangOpts().CPlusPlus11 || S.getLangOpts().C99) { const Stmt *Term = B->getTerminatorStmt(); a

[PATCH] D56366: New warning call-to-virtual-from-ctor-dtor when calling a virtual function from a constructor or a destructor

rjmccall added inline comments. Comment at: lib/Sema/SemaOverload.cpp:13075 + if ((isa(CurContext) || + isa(CurContext)) && + isa(MemExpr->getBase()->IgnoreParenCasts()) && Intentionally suppressed in lambdas? I think that might be reasonable, but a

[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: > > Anastas

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

rjmccall added inline comments. Comment at: include/clang/Sema/Sema.h:2124 +// Implicitly initialized subobject. +NTCUC_ImplicitInitSubObject, +// Uninialized variable with automatic storage duration. "default-initialized", please.

[PATCH] D56366: New warning call-to-virtual-from-ctor-dtor when calling a virtual function from a constructor or a destructor

rjmccall added a comment. Oh, sorry, I had completely forgotten about that. My contributions to Clang are primarily code review these days; I am not working on that idea. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56366/new/ https://reviews.llvm.org/D56366

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:16218 +checkNonTrivialCUnion(E->getType(), E->getExprLoc(), + Sema::NTCUC_LValueToRValueVolatile); + ahatanak wrote: > rjmccall wrote: > > It looks like you're generall

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

rjmccall added a comment. Okay. In that case, yeah, it'd be good to get Richard's opinion about the right way to get that information here. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58164/new/ https://reviews.llvm.org/D58164 _

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

rjmccall added a comment. Thanks, looks much better. Comment at: include/clang/Sema/Sema.h:2066 bool DeduceVariableDeclarationType(VarDecl *VDecl, bool DirectInit, - Expr *Init); + Expr *Init, bool Defau

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