[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:17183 + class DeferredDiagnosticsEmitter + : public EvaluatedExprVisitor { +Sema &S; Is there any way to share most of the visitation logic here with the visitor we use in `MarkDec

[PATCH] D44559: [Sema] Wrong width of result of mul operation

rjmccall added a comment. In https://reviews.llvm.org/D44559#1044186, @avt77 wrote: > >> In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote: > >> > >>> I think we're correct not to warn here and that GCC/ICC are being noisy. > >>> The existence of a temporary promotion to a wider type

[PATCH] D44559: [Sema] Wrong width of result of mul operation

rjmccall added a comment. In https://reviews.llvm.org/D44559#1044653, @aaron.ballman wrote: > In https://reviews.llvm.org/D44559#1044639, @rjmccall wrote: > > > In https://reviews.llvm.org/D44559#1044186, @avt77 wrote: > > > > > >> In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote: > >

[PATCH] D44747: [AMDGPU] Set calling convention for CUDA kernel

rjmccall added a comment. Is there a reason for this to be done as a special case in IRGen instead of just implicitly applying the calling convention in Sema? https://reviews.llvm.org/D44747 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

[PATCH] D44747: [AMDGPU] Set calling convention for CUDA kernel

rjmccall added a comment. In https://reviews.llvm.org/D44747#1044916, @yaxunl wrote: > In https://reviews.llvm.org/D44747#1044893, @rjmccall wrote: > > > Is there a reason for this to be done as a special case in IRGen instead of > > just implicitly applying the calling convention in Sema? > > >

[PATCH] D44559: [Sema] Wrong width of result of mul operation

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. No, I still oppose this patch. https://reviews.llvm.org/D44559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

rjmccall added a comment. We added the `unsafe_unretained` property attribute as part of ARC because we were introducing `__unsafe_retained` as a type qualifier and we wanted all the type qualifiers to have corresponding attribute spellings. `assign` is the much-older attribute, and its non-ow

[PATCH] D44747: Set calling convention for CUDA kernel

rjmccall added inline comments. Comment at: lib/AST/Type.cpp:2762 + case CC_CUDAKernel: +return "cuda_kernel"; } For consistency with the rest of this switch, please put the return on the same line as its case. Comment at: lib/AST/Type

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

rjmccall added a comment. I'm not sure you really need to put these in their own warning sub-group just because they're user-defined operators. That's especially true because it appears we already have divisions in the warning group based on the form of the l-value; we don't want this to go co

[PATCH] D44559: [Sema] Wrong width of result of mul operation

rjmccall added a comment. In https://reviews.llvm.org/D44559#1048085, @avt77 wrote: > In https://reviews.llvm.org/D44559#1045758, @rjmccall wrote: > > > No, I still oppose this patch. > > > OK, we have 2 possibilities here (fmpov): > > 1. Forget about the issue and don't do anything now - it is n

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

rjmccall added a comment. In https://reviews.llvm.org/D44883#1048081, @lebedev.ri wrote: > In https://reviews.llvm.org/D44883#1048010, @rjmccall wrote: > > > I'm not sure you really need to put these in their own warning sub-group > > just because they're user-defined operators. That's especial

[PATCH] D44559: [Sema] Wrong width of result of mul operation

rjmccall added a comment. In https://reviews.llvm.org/D44559#1048399, @lebedev.ri wrote: > I'm having a very hard time following this thread. In fact, i can't follow > the logic at all. > > In https://reviews.llvm.org/D44559#1041007, @rjmccall wrote: > > > ... > > This patch, however, is contra

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

rjmccall added a comment. In https://reviews.llvm.org/D44883#1048439, @lebedev.ri wrote: > In https://reviews.llvm.org/D44883#1048421, @Quuxplusone wrote: > > > Two more high-level comments from me, and then I'll try to butt out. > > > > Q1. I don't understand what `field-` is doing in the name o

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

rjmccall added a comment. In https://reviews.llvm.org/D44883#1048485, @rjmccall wrote: > In https://reviews.llvm.org/D44883#1048439, @lebedev.ri wrote: > > > In https://reviews.llvm.org/D44883#1048400, @rjmccall wrote: > > > > > Yeah, like I said, I'm just worried about the usability of having >

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

rjmccall added a comment. Is D not considered trivial there? Ugh, C++. Well, we certainly could justify treating D as a builtin operator as well, since it doesn't involve running any user code, but I don't think you're obligated to write a whole bunch of analysis just to make that work if the

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

rjmccall added a comment. I tracked Chandler down, and he doesn't remember why user-defined operators are excluded. Repository: rC Clang https://reviews.llvm.org/D44883 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.or

[PATCH] D44908: [ObjC++] Make parameter passing and function return compatible with ObjC

rjmccall added a comment. I think it's reasonable to pull that into LangOpts. It would need to be in LangOpts anyway if we e.g. fixed an ABI bug in struct layout and needed a compatibility handling to preserve the old behavior. Repository: rC Clang https://reviews.llvm.org/D44908 __

[PATCH] D44913: [ObjC] Enable using C++ triviality type traits for non-trivial C structs

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

[PATCH] D44916: [Sema] Avoid crash for category implementation without interface

rjmccall added a comment. I think that's a fine place to add the check. Repository: rC Clang https://reviews.llvm.org/D44916 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44908: [ObjC++] Make parameter passing and function return compatible with ObjC

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

[PATCH] D44559: [Sema] Wrong width of result of mul operation

rjmccall added a comment. In https://reviews.llvm.org/D44559#1049049, @avt77 wrote: > > If that operation overflows, so be it — we're not going to warn about the > > potential for overflow every time the user adds two ints, and by the same > > token, it doesn't make any sense to warn about ever

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:12093 +break; + } + I think doing this here can result in double-warning if the overload resolves to a builtin operator. Now, it might not actually be possible for that to combine with the

[PATCH] D44747: Set calling convention for CUDA kernel

rjmccall added inline comments. Comment at: lib/Sema/SemaOverload.cpp:1492 + Changed = true; +} + yaxunl wrote: > rjmccall wrote: > > It's cheaper not to check the CUDA language mode here; pulling the CC out > > of the FPT is easy. > > > > Why is this

[PATCH] D44968: [ObjC] Generalize NRVO to cover non-trivial C structs

rjmccall added a comment. Is it possible to just do this for all structs? I don't think it hurts anything to do it for structs that are trivial and returned indirectly, and it would certainly be nice to construct C return values in-place even if they're guilty of nothing more than being, y'kno

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:12093 +break; + } + lebedev.ri wrote: > rjmccall wrote: > > I think doing this here can result in double-warning if the overload > > resolves to a builtin operator. Now, it might not actuall

[PATCH] D44747: Set calling convention for CUDA kernel

rjmccall added a comment. LGTM. If `__global__` is supported in C++ structures, you might also need to make sure that member function constants (`&A::kernel_function`) drop the CC. And it might be a good idea to make sure that `decltype(kernel_function)` doesn't have a problem with it, either

[PATCH] D44985: Remove initializer for CUDA shared varirable

rjmccall added a comment. What exactly are you trying to express here? Are you just trying to make these external declarations when compiling for the device because `__shared__` variables are actually defined on the host? That should be handled by the frontend by setting up the AST so that th

[PATCH] D44987: Disable emitting static extern C aliases for amdgcn target for CUDA

rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenModule.cpp:4684 + (Context.getTargetInfo().getTriple().getArch() == llvm::Triple::amdgcn)) return; for (auto &I : StaticExternCValues) { Please add a target hook for this instead of build

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

rjmccall added a comment. You should send an RFC to cfe-dev about adding this new language mode. I understand that it's very similar to an existing language mode that we already support, and that's definitely we'll consider, but we shouldn't just agree to add new language modes in patch review

[PATCH] D44985: Remove initializer for CUDA shared varirable

rjmccall added a comment. In https://reviews.llvm.org/D44985#1050674, @yaxunl wrote: > In https://reviews.llvm.org/D44985#1050670, @rjmccall wrote: > > > What exactly are you trying to express here? Are you just trying to make > > these external declarations when compiling for the device becaus

[PATCH] D44987: Disable emitting static extern C aliases for amdgcn target for CUDA

rjmccall accepted this revision. rjmccall added a comment. LGTM. https://reviews.llvm.org/D44987 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44747: Set calling convention for CUDA kernel

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

[PATCH] D44968: [ObjC] Generalize NRVO to cover C structs

rjmccall added a comment. Wow, the IR improvements here are amazing. Comment at: lib/CodeGen/CGDecl.cpp:1119 +if ((CXXRD && !CXXRD->hasTrivialDestructor()) || +RD->isNonTrivialToPrimitiveCopy()) { // Create a flag that is used to indicate when the

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:12093 +break; + } + lebedev.ri wrote: > rjmccall wrote: > > lebedev.ri wrote: > > > rjmccall wrote: > > > > I think doing this here can result in double-warning if the overload > > > > resolv

[PATCH] D44968: Generalize NRVO to cover C structs

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

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:12093 +break; + } + lebedev.ri wrote: > rjmccall wrote: > > lebedev.ri wrote: > > > rjmccall wrote: > > > > lebedev.ri wrote: > > > > > rjmccall wrote: > > > > > > I think doing this here can

[PATCH] D44536: Avoid segfault when destructor is not yet known

rjmccall added a comment. I think it's part of an effort to avoid creating implicit declarations for all the special members of every struct we parse from system headers. Repository: rC Clang https://reviews.llvm.org/D44536 ___ cfe-commits maili

[PATCH] D44536: Avoid segfault when destructor is not yet known

rjmccall added a comment. In https://reviews.llvm.org/D44536#1051181, @ahatanak wrote: > I see, so Sema::CheckCompletedCXXClass probably isn't the right place to call > DeclareImplicitDestructor as that could significantly increase the size of > the AST. Right. Again, I'd like Richard to wei

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

rjmccall added a comment. In https://reviews.llvm.org/D44883#1051878, @lebedev.ri wrote: > Ok, tried llvm stage2 build, and it failed as expected, in code that > intentionally does self-assignment: Actually, this is not the sort of failure that I think should worry you. Any new warning has t

[PATCH] D44985: Remove initializer for CUDA shared varirable

rjmccall added a comment. In https://reviews.llvm.org/D44985#1051840, @yaxunl wrote: > In https://reviews.llvm.org/D44985#1050876, @rjmccall wrote: > > > In https://reviews.llvm.org/D44985#1050674, @yaxunl wrote: > > > > > In https://reviews.llvm.org/D44985#1050670, @rjmccall wrote: > > > > > > >

[PATCH] D44559: [Sema] Wrong width of result of mul operation

rjmccall added a comment. What would the design for that warning be? C promotes all arithmetic on sub-int types to int, and if that's assigned back to a variable of the original type, there's a conversion. But you seem to only want to warn about truncating the result of multiplication and not

[PATCH] D44985: [CUDA] Let device-side shared variables be initialized with undef

rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:235-240 + if (Ty.getAddressSpace() != LangAS::opencl_local && + !(getLangOpts().CUDA && getLangOpts().CUDAIsDevice && +D.hasAttr())) Init = EmitNullConstant(Ty); else Init = llvm::UndefV

[PATCH] D45101: [ObjC] Use the name specified by objc_runtime_name instead of the class identifier

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D45101#1053246, @ahatanak wrote: > Note that CGObjCNonFragileABIMac::EmitClassRef also passes the class > identifier to CGObjCNonFragileABIMac::EmitClassRefFrom

[PATCH] D44238: [CFG] Fix automatic destructors when a member is bound to a reference.

rjmccall added a comment. Seems fine to me, but you might want someone with analyzer experience to review. Repository: rC Clang https://reviews.llvm.org/D44238 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

rjmccall added inline comments. Comment at: docs/ReleaseNotes.rst:63 +- ``-Wself-assign`` and ``-Wself-assign-field`` were extended to diagnose + self-assignment operations using overloaded operators (i.e. classes) + Missing a final period. Also, these release

[PATCH] D45112: [MS] Emit vftable thunks for functions with incomplete prototypes

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

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks. LGTM whenever you've cleared up the self-hosting problems. Repository: rC Clang https://reviews.llvm.org/D44883 ___ cfe-commits m

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

rjmccall added a comment. Right. I think it's fair to acknowledge that many data structure unit tests will contain a legitimate use of a user-defined self-assignment without feeling that that disqualifies the warning. Note that the purpose of this kind of breadth testing is just to look for fa

[PATCH] D44985: [CUDA] Let device-side shared variables be initialized with undef

rjmccall accepted this revision. rjmccall added a comment. LGTM, thanks. https://reviews.llvm.org/D44985 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45163: [Sema] -Wunused-value: diagnose unused std::move() call results.

rjmccall added a comment. LGTM. I think it wouldn't be unreasonable to ask standard library maintainers to add `[[nodiscard]]` to `std::move`, but it's also not unreasonable for us to special-case some functions. Repository: rC Clang https://reviews.llvm.org/D45163 _

[PATCH] D45163: [Sema] -Wunused-value: diagnose unused std::move() call results.

rjmccall added a comment. Yeah, actually, I'm second-guessing myself. Maybe this should just be a libc++ / libstdc++ bug. Repository: rC Clang https://reviews.llvm.org/D45163 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.

[PATCH] D45163: [Sema] -Wunused-value: diagnose unused std::move() call results.

rjmccall added a comment. That seems reasonable. And this summer would still be in time for the next release. Repository: rC Clang https://reviews.llvm.org/D45163 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

rjmccall added a comment. No, the analysis is intentionally syntactic; it should apply even on dependently-typed arguments (but not get re-checked on instantiation). But I agree that the warning ought to be disabled in unevaluated contexts. Repository: rC Clang https://reviews.llvm.org/D44

[PATCH] D44536: Avoid segfault when destructor is not yet known

rjmccall added a comment. In https://reviews.llvm.org/D44536#1054929, @rsmith wrote: > It seems that we have two options: either we valiantly try to support this: > > - we keep a list of types for which we've tried to form a > //delete-expression//, but found that the type was incomplete > - whe

[PATCH] D45163: [Sema] -Wunused-value: diagnose unused std::move() call results.

rjmccall added a comment. Well, we should feel welcome to submit patches to the C++ library implementations, I think. I can understand why Marshall is waiting to just do this until he gets a comprehensive committee paper, but he might consider taking a patch in the meantime. But I'm not sure

[PATCH] D45163: [Sema] -Wunused-value: diagnose unused std::move() call results.

rjmccall added a comment. Trust me, I understand that this is an important function, but a lot of functions are important, and we're not going to hardcode knowledge about all of them in the compiler. It seems reasonable to ask libc++ to use `__attribute__((warn_unused_result))` if `[[nodiscard

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

rjmccall added a comment. Is Marshall arguing that the standard doesn't allow compilers to warn about failing to use these function results prior to C++17? Because I don't think that's true; warnings are thoroughly non-normative. If libc++ wants to allow its users to opt out of these warnings

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

rjmccall added a comment. I think the appropriate place to do this is in IsStandardConversion, immediately after the call to ResolveAddressOfOverloadedFunction. You might want to add a general utility for getting the type-of-reference of a function decl. https://reviews.llvm.org/D45223 __

[PATCH] D45176: implement recent "standard-layout" changes

rjmccall added inline comments. Comment at: AST/DeclCXX.cpp:1130 data().IsStandardLayout = false; + data().IsCXX11StandardLayout = false; +} `IsCXX11StandardLayout` should be based on `FieldRec->isCXX11StandardLayout()`, I assume. h

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

rjmccall added a comment. Yes, if we think that the committee is likely to include questionable functions on the `[[nodiscard]]` list which we don't want to warn about pre-C++17, then it makes sense to have two internal macros, one for functions like `std::move` that should be unconditionally w

[PATCH] D45176: implement recent "standard-layout" changes

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay. LGTM. Thank you for putting the effort into maintaining both rules simultaneously. https://reviews.llvm.org/D45176 ___ cfe-commits m

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, LGTM with the reduced set of changes to the functionality. Repository: rC Clang https://reviews.llvm.org/D44580 ___ cfe-commits mail

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

rjmccall added a comment. The other changes I see here seem reasonable, but please do split the patch. Comment at: include/clang/Basic/Cuda.h:61 + GFX900, + GFX902, LAST, Does this actually have anything to do with HIP? You have a lot of changes in this

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

rjmccall added inline comments. Comment at: include/clang/Basic/Cuda.h:61 + GFX900, + GFX902, LAST, yaxunl wrote: > rjmccall wrote: > > Does this actually have anything to do with HIP? You have a lot of changes > > in this patch which seem to just be about

[PATCH] D45277: [CUDA] Add amdgpu sub archs

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks for splitting the patch up. LGTM. https://reviews.llvm.org/D45277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://list

[PATCH] D45289: Disable -fmerge-all-constants as default.

rjmccall added a comment. It's not unreasonable to just add -fmerge-all-constants to the command line invocations for the individual tests, yeah. We can take those off as necessary later. Repository: rC Clang https://reviews.llvm.org/D45289 __

[PATCH] D45289: Disable -fmerge-all-constants as default.

rjmccall added inline comments. Comment at: include/clang/Driver/Options.td:1286 def fno_merge_all_constants : Flag<["-"], "fno-merge-all-constants">, Group, Flags<[CC1Option]>, HelpText<"Disallow merging of constants">; def fno_modules : Flag <["-"], "fno-modules">, Grou

[PATCH] D45305: ObjCGNU: Fix empty v3 protocols being emitted two fields short

rjmccall added a comment. The change LGTM, but please add a test. Repository: rC Clang https://reviews.llvm.org/D45305 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42092: implement C++ dr388 for the Itanium C++ ABI: proper handling of catching exceptions by reference

rjmccall added a comment. In https://reviews.llvm.org/D42092#1057881, @rsmith wrote: > In https://reviews.llvm.org/D42092#983892, @rjmccall wrote: > > > This is definitely something that the personality function should handle. > > If we want to do heroic things in the absence of personality fun

[PATCH] D45306: PR36992 don't overwrite virtual bases in tail padding

rjmccall added a comment. Changing the requirements on the return-value slot would be an ABI break, no? And it would force to us to treat all complete-object constructions as nvsize-limited unless they're constructions of known sizeof-sized allocations. Comment at: CodeGen/C

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

rjmccall added inline comments. Comment at: include/clang/AST/NonTrivialCStructTypeVisitor.h:12 +// +//===--===// + The header comment here was clearly just copied from another file. I would nam

[PATCH] D45306: PR36992 don't overwrite virtual bases in tail padding

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

[PATCH] D42092: implement C++ dr388 for the Itanium C++ ABI: proper handling of catching exceptions by reference

rjmccall added a comment. In https://reviews.llvm.org/D42092#1058841, @rsmith wrote: > In https://reviews.llvm.org/D42092#1058772, @rjmccall wrote: > > > Issue #3 is tricky; it's arguably not okay to force a landing at a landing > > pad if we're not actually catching anything. > > > I think the

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

rjmccall added inline comments. Comment at: include/clang/AST/NonTrivialCStructTypeVisitor.h:30 +if (asDerived().getContext().getAsArrayType(FT)) + return asDerived().visitArray(DK, FT, std::forward(Args)...); + ahatanak wrote: > rjmccall wrote: > > Shou

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

rjmccall added a comment. Hmm. I'm not actually sure *why* it's not okay to forward callee-cleanup arguments. Do we just not have the necessary logic to disable the cleanup in the caller? Repository: rC Clang https://reviews.llvm.org/D45382 _

[PATCH] D45384: [ObjC++] Never pass structs with `__weak` fields in registers

rjmccall added a comment. Well, but I think CanPassInRegisters==false in the base class does always mean CanPassInRegisters==false in the subclass. Repository: rC Clang https://reviews.llvm.org/D45384 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D44616: Update existed CodeGen TBAA tests

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Yes, this is fine. Repository: rC Clang https://reviews.llvm.org/D44616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lis

[PATCH] D45384: [ObjC++] Never pass structs with `__weak` fields in registers

rjmccall added a comment. In https://reviews.llvm.org/D45384#1060192, @ahatanak wrote: > In https://reviews.llvm.org/D45384#1060164, @rjmccall wrote: > > > Well, but I think CanPassInRegisters==false in the base class does always > > mean CanPassInRegisters==false in the subclass. > > > I think

[PATCH] D45384: [ObjC++] Never pass structs with `__weak` fields in registers

rjmccall added a comment. In https://reviews.llvm.org/D45384#1060369, @ahatanak wrote: > Yes. I intended it as a property that propagates to classes that contain or > derive from the type. > > Would it make it less confusing if I merged CXXRecordDecl::CanPassInRegisters > and RecordDecl::Cannot

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

rjmccall added a comment. This looks okay to me, but I think it would better if someone with more expertise in the design of the driver and frontend code could review this. Comment at: lib/Driver/Driver.cpp:2267 +if ((IA->getType() != types::TY_CUDA) && +IA

[PATCH] D45384: [ObjC++] Never pass structs with `__weak` fields in registers

rjmccall added a comment. Just a couple minor requests; if you accept them, feel free to commit. Comment at: include/clang/AST/Decl.h:3556 +/// indirectly. This value is used only in C++. +APK_CannotPassInRegs, + I think it's probably worth spelling out

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

rjmccall added a comment. If that's the problem, then I think the right design is for CallArg to include an optional cleanup reference for a cleanup that can be deactivated at the instant of the call (we should assert that this exists for parameters that are destroyed in the callee), and then f

[PATCH] D45305: ObjCGNU: Fix empty v3 protocols being emitted two fields short

rjmccall accepted this revision. rjmccall added a comment. Hmm. Alright, I guess. Repository: rC Clang https://reviews.llvm.org/D45305 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

rjmccall closed this revision. rjmccall added a comment. Committed as r329508. Repository: rC Clang https://reviews.llvm.org/D44580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45411: [Sema] Fix PR35832 - Ambiguity accessing anonymous struct/union with multiple bases.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Well, that is a really silly bug. Fix LGTM. https://reviews.llvm.org/D45411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://l

[PATCH] D45412: [Sema] Fix PR22637 - IndirectFieldDecl's discard qualifiers during template instantiation.

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

[PATCH] D45410: [Sema] Remove dead code in BuildAnonymousStructUnionMemberReference. NFCI

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I think there was a point when we weren't always creating CXXThisExprs eagerly for these accesses. Now that we are, yeah, this should be dead. Repository: rC Clang https://reviews.llv

[PATCH] D39138: [CodeGen] Generate TBAA info for 'this' pointers

rjmccall added a comment. In https://reviews.llvm.org/D39138#906623, @kosarev wrote: > Hmm, according to our research such loads constitute about 18% of all loads > under ##-O -Xclang -disable-llvm-passes## on the LLVM code base. I wonder, do > you think it would be nice to not generate them at

[PATCH] D38796: [CodeGen] EmitPointerWithAlignment() to generate TBAA info along with LValue base info

rjmccall added a comment. I think probably the best solution is to track may-alias-ness explicitly in the TBAAAccessInfo instead of relying in the frontend on being able to distinguish things in the LLVM metadata. Repository: rL LLVM https://reviews.llvm.org/D38796 __

[PATCH] D39305: Simplify codegen and debug info generation for block context parameters.

rjmccall added a comment. The original code doesn't make any sense; it looks like the indirection is backwards. We just built a debug variable corresponding to the block descriptor pointer parameter, so LocalAddr should be dbg.declare'd to be that variable and Arg should be dbg.value'd. It lo

[PATCH] D38774: [CodeGen] Add support for IncompleteArrayType in Obj-C ivars.

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

[PATCH] D39177: [CodeGen] Generate TBAA info for reference loads

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

[PATCH] D39008: [CodeGen] Propagate may-alias'ness of lvalues with TBAA info

rjmccall added a comment. Sure, that makes sense to me. John. Repository: rL LLVM https://reviews.llvm.org/D39008 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39177: [CodeGen] Generate TBAA info for reference loads

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Looks good; sorry for not catching that, either. https://reviews.llvm.org/D39177 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http

[PATCH] D39374: CodeGen: Fix insertion position of addrspace cast for alloca

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

[PATCH] D39008: [CodeGen] Propagate may-alias'ness of lvalues with TBAA info

rjmccall added a comment. Looks good, but you missed updating equality/hashing for the new Kind field. Comment at: lib/CodeGen/CodeGenTBAA.h:71 AccessType == Other.AccessType && Offset == Other.Offset; } This needs to factor in the Ki

[PATCH] D39008: [CodeGen] Propagate may-alias'ness of lvalues with TBAA info

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

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

rjmccall added a comment. The actual choice of integer type is not stable across targets any more than the size is. In practice, people often don't use int and long, they use standard typedefs like size_t and uint64_t, but the actual type chosen for size_t is arbitrary when there are multiple

[PATCH] D39505: [OpenMP] Show error if VLAs are not supported

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:944 + /// \brief Whether target supports variable-length arrays. + bool isVLASupported() const { return VLASupported; } + The way you've written this makes it sound like "does the targ

[PATCH] D39505: [OpenMP] Show error if VLAs are not supported

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:944 + /// \brief Whether target supports variable-length arrays. + bool isVLASupported() const { return VLASupported; } + Hahnfeld wrote: > rjmccall wrote: > > The way you've written t

<    22   23   24   25   26   27   28   29   30   31   >