Page MenuHomePhabricator

rjmccall (John McCall)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2013, 11:30 AM (330 w, 3 d)

Recent Activity

Yesterday

rjmccall added a comment to D61808: [ObjC] Replace uses of the argument of a call to objc_autorelease with the result in MRR.

Right, exactly.

Mon, May 20, 1:34 PM · Restricted Project
rjmccall added a comment to D61808: [ObjC] Replace uses of the argument of a call to objc_autorelease with the result in MRR.

No, because hasCustomRR is set whenever the method implementation isn't the standard -[NSObject autorelease] implementation. In other words, this is just inlining the standard implementation when it's dynamically known that the dispatch would end up there anyway.

Mon, May 20, 1:11 PM · Restricted Project
rjmccall added inline comments to D62156: [Sema] Diagnose addr space mismatch while constructing objects.
Mon, May 20, 1:00 PM
rjmccall added a comment to D61808: [ObjC] Replace uses of the argument of a call to objc_autorelease with the result in MRR.

I don't know what you mean. Currently, if the callee tail-calls autorelease (and necessarily this has to be MRR code), we perform an ordinary message send of autorelease. It happens to be the case that the standard implementation of autorelease will allow the autorelease to be reclaimed, but nothing about that choice is in any way non-standard on the MRR side.

Mon, May 20, 12:38 PM · Restricted Project

Thu, May 16

rjmccall added a comment to D61808: [ObjC] Replace uses of the argument of a call to objc_autorelease with the result in MRR.

Oh, yes, that's correct, at least under base ObjC rules.

Thu, May 16, 4:01 PM · Restricted Project
rjmccall added a comment to D61808: [ObjC] Replace uses of the argument of a call to objc_autorelease with the result in MRR.

The assumption that these methods return the receiver is not true without additional semantic assumptions about these methods, so the current behavior of unconditionally turning [x autorelease] into objc_autorelease(x), x seems quite broken.

Because it's possible to override an autorelease method and make the method return something other than the passed argument, for example?

If it's not possible to assume that the method returns the passed argument in MRR, the direction of this patch and https://reviews.llvm.org/D61970 is completely wrong.

Thu, May 16, 3:20 PM · Restricted Project
rjmccall added a comment to D59628: Add support for __attribute__((objc_class_stub)).

Thanks! Down to minor stuff now.

Thu, May 16, 1:27 PM · Restricted Project
rjmccall added inline comments to D60456: [RISCV][WIP/RFC] Hard float ABI support.
Thu, May 16, 10:32 AM · Restricted Project
rjmccall added a comment to D61808: [ObjC] Replace uses of the argument of a call to objc_autorelease with the result in MRR.

The assumption that these methods return the receiver is not true without additional semantic assumptions about these methods, so the current behavior of unconditionally turning [x autorelease] into objc_autorelease(x), x seems quite broken.

Thu, May 16, 10:14 AM · Restricted Project
rjmccall added inline comments to D61970: [CodeGen][ObjC] Call objc_autoreleaseReturnValue and objc_retainAutoreleasedReturnValue instead of objc_autorelease and objc_retain in MRR.
Thu, May 16, 9:51 AM · Restricted Project

Fri, May 10

rjmccall added inline comments to D61788: Make getObjCEncodingForTypeImpl() take a bitmask instead of 8 bools.
Fri, May 10, 12:51 PM · Restricted Project
rjmccall added inline comments to D61788: Make getObjCEncodingForTypeImpl() take a bitmask instead of 8 bools.
Fri, May 10, 10:44 AM · Restricted Project

Thu, May 9

rjmccall accepted D61667: Assume `__cxa_allocate_exception` returns an under-aligned memory on Darwin if the version of libc++abi isn't new enough to include the fix in r319123.

Thanks, LGTM.

Thu, May 9, 5:52 PM · Restricted Project
rjmccall accepted D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

LGTM.

Thu, May 9, 5:51 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D61667: Assume `__cxa_allocate_exception` returns an under-aligned memory on Darwin if the version of libc++abi isn't new enough to include the fix in r319123.
Thu, May 9, 4:43 PM · Restricted Project
rjmccall added inline comments to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.
Thu, May 9, 4:34 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.
Thu, May 9, 4:07 PM · Restricted Project, Restricted Project

Wed, May 8

rjmccall added inline comments to D61667: Assume `__cxa_allocate_exception` returns an under-aligned memory on Darwin if the version of libc++abi isn't new enough to include the fix in r319123.
Wed, May 8, 2:25 PM · Restricted Project
rjmccall added inline comments to D61667: Assume `__cxa_allocate_exception` returns an under-aligned memory on Darwin if the version of libc++abi isn't new enough to include the fix in r319123.
Wed, May 8, 12:14 PM · Restricted Project
rjmccall added inline comments to D61667: Assume `__cxa_allocate_exception` returns an under-aligned memory on Darwin if the version of libc++abi isn't new enough to include the fix in r319123.
Wed, May 8, 12:08 PM · Restricted Project

Tue, May 7

rjmccall added inline comments to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.
Tue, May 7, 8:00 PM · Restricted Project, Restricted Project
rjmccall added a comment to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

All of the IRGen changes in this patch are unnecessary according to the model we've worked out, right? The fix is just to mark the destructor as still used when we're constructing an array.

Tue, May 7, 3:12 PM · Restricted Project, Restricted Project
rjmccall added a comment to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

For the purposes of this patch, I think that means we never need a destructor for the type of a [[no_destroy]] variable.

Arrays and other subobjects of an aggregate initializaton, unless applying the standard "literally" implies the obviously perverse result that we don't destroy subobjects in such cases.

Yes, but you need the destructors for those subobjects as a side-condition of the initialization, irrespective of what kind of object that initialization is initializing. I don't think that's got anything to do with [[no_destroy]]. I think it remains the case that you never need a destructor for the type of a [[no_destroy]] variable.

Tue, May 7, 2:49 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D59988: [PR41276] Generate address space cast of 'this' for objects attributed by an address space in C++.
Tue, May 7, 10:50 AM · Restricted Project

Mon, May 6

rjmccall added a comment to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

I think the intuitive rule is that initialization is complete when the full-expression performing the initialization is complete because that's the normal unit of sequencing. Note that the standard does use both "constructed" and "initialized" in different contexts, although this might just be an editorial choice.

I think it would be quite subtle if the standard was deliberately splitting this particular hair by implicitly creating a "constructed, but not initialized" state of an object, without calling that out anywhere :)

If by "constructed" you mean "a constructor has finished", then we need to split this hair. Consider:

struct B { ~B(); };
struct A {
  A() {}
  A(B &&) : A() { throw 0; }
};
void f() {
  static A a = B();
}

At the point when the exception is thrown, a constructor for a has completed, but its initialization is not complete.

[except.ctor]/3 and /4 lay out the rules:

"""
If the initialization or destruction of an object other than by delegating constructor is terminated by an
exception, the destructor is invoked for each of the object’s direct subobjects and, for a complete object,
virtual base class subobjects, whose initialization has completed (9.3) and whose destructor has not yet begun
execution, except that in the case of destruction, the variant members of a union-like class are not destroyed.
[Note: If such an object has a reference member that extends the lifetime of a temporary object, this ends
the lifetime of the reference member, so the lifetime of the temporary object is effectively not extended.
— end note] The subobjects are destroyed in the reverse order of the completion of their construction. Such
destruction is sequenced before entering a handler of the function-try-block of the constructor or destructor,
if any.

If the compound-statement of the function-body of a delegating constructor for an object exits via an
exception, the object’s destructor is invoked. Such destruction is sequenced before entering a handler of the
function-try-block of a delegating constructor for that object, if any.
"""

The above wording seems to suggest that the initialization of an object of class type completes when its outermost constructor finishes (at least for the case of initialization by constructor). And indeed all the other wording I can find that has some bearing on when an object is deemed initialized suggests that option 1 is correct, and in general that the initialization of a variable is complete when the initialization full-expression ends, which is before the destructors for any temporaries run. (Those destructor calls are separate full-expressions that happen afterwards; see [intro.execution]/5.)

Mon, May 6, 8:03 PM · Restricted Project, Restricted Project
rjmccall added a comment to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which appears to claim that initialization ends with the execution of the constructor, excluding temporary destructors.

(13) In a non-delegating constructor, initialization proceeds in the following order:
/* list of steps... */
(13.4) Finally, the compound-statement of the constructor body is executed.

John: do you agree with this analysis?

That's talking about constructor bodies.

Sure, but that's all that dcl.int suggests happens during initialization. Skipping over paragraphs that don't apply:

http://eel.is/c++draft/dcl.init#17: The semantics of initializers are as follows.

  • http://eel.is/c++draft/dcl.init#17.6: [I]f the destination type is a (possibly cv-qualified) class type:
    • http://eel.is/c++draft/dcl.init#17.6.2: [I]f the initialization is direct-initialization, or if it is copy-initialization where the cv-unqualified version of the source type is the same class as, or a derived class of, the class of the destination, constructors are considered. The applicable constructors are enumerated ([over.match.ctor]), and the best one is chosen through overload resolution ([over.match]). Then:
      • http://eel.is/c++draft/dcl.init#17.6.2.1: If overload resolution is successful, the selected constructor is called to initialize the object, with the initializer expression or expression-list as its argument(s).
Mon, May 6, 7:35 PM · Restricted Project, Restricted Project
rjmccall added a comment to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which appears to claim that initialization ends with the execution of the constructor, excluding temporary destructors.

(13) In a non-delegating constructor, initialization proceeds in the following order:
/* list of steps... */
(13.4) Finally, the compound-statement of the constructor body is executed.

John: do you agree with this analysis?

Mon, May 6, 5:01 PM · Restricted Project, Restricted Project

Fri, May 3

rjmccall added a comment to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

The flip side of that argument is that (1) there aren't very many users right now and (2) it's much easier to start conservative and then weaken the rule than it will be to strengthen it later. It really isn't acceptable to just turn off access/use-checking for the destructor, so if we get trapped by the choice we make here, we'll end up having to either leak or call std::terminate.

Fri, May 3, 4:46 PM · Restricted Project, Restricted Project
rjmccall added a comment to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

I think the intuitive rule is that initialization is complete when the full-expression performing the initialization is complete because that's the normal unit of sequencing. Note that the standard does use both "constructed" and "initialized" in different contexts, although this might just be an editorial choice.

I think it would be quite subtle if the standard was deliberately splitting this particular hair by implicitly creating a "constructed, but not initialized" state of an object, without calling that out anywhere :)

Fri, May 3, 3:11 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D59988: [PR41276] Generate address space cast of 'this' for objects attributed by an address space in C++.
Fri, May 3, 1:01 PM · Restricted Project
rjmccall added a comment to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

That's only true for subobjects of an enclosing aggregate before that aggregate's initialization is complete though, right? So it doesn't seem like that much of an inconsistency, just mimicking what we would be doing if an exception was thrown in, say, the body of the ctor before the object's initialization is completed.

Conceptually yes, but formally no. The standard *could* write this rule as "all currently-initialized subobjects must be separately destroyed when an exception aborts initialization of the containing aggregate, including constructor bodies and aggregate initialization", but it doesn't actually do so; instead it has specific rules covering the behavior when an exception is thrown out of the body of a constructor, and those rules simply do not apply to aggregate initialization.

Right, I was just trying to draw an analogy. Can you be more specific about the inconsistency you mentioned above? What objects with static storage duration have to be destroyed when an exception is thrown? Just subobjects of static aggregates that had their initialization aborted by an exception? If so, that obviously doesn't seem inconsistent.

Fri, May 3, 10:59 AM · Restricted Project, Restricted Project
rjmccall accepted D61318: [Sema] Prevent binding references with mismatching address spaces to temporaries.

LGTM.

Fri, May 3, 9:24 AM
rjmccall accepted D60454: [OpenCL] Prevent mangling kernel functions.

LGTM.

Fri, May 3, 9:21 AM · Restricted Project

Thu, May 2

rjmccall added a comment to D61458: [hip] Relax CUDA call restriction within `decltype` context..

Here's one for you:

__host__ float bar();
__device__ int bar();
__host__ __device__ auto foo() -> decltype(bar()) {}

What is the return type of foo? :)

I don't believe the right answer is, "float when compiling for host, int when compiling for device."

So, actually, I wonder if that's not the right answer. We generally allow different overloads to have different return types.

Thu, May 2, 7:55 PM · Restricted Project
rjmccall added inline comments to D61454: [CodeGen][ObjC] Remove the leading 'l_' from ObjC symbols and make private symbols in the __DATA segment internal..
Thu, May 2, 3:58 PM · Restricted Project
rjmccall added a comment to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

It seems like the most common sense interpretation here is to just treat the initialization of G as completed at the point when the constructor finishes (this appears to be what GCC implements: https://wandbox.org/permlink/R3C9pPhoT4efAdL1).

Your example doesn't actually demonstrate that that's what GCC implements because it doesn't try to execute the declaration twice. But you're right, GCC does appear to consider the initialization complete as soon as the static object's constructor returns normally. On the other hand, GCC gets the array case here wrong: if a static local has array type, and a destructor for a temporary required by the first element initializer throws, then it does not destroy the element but also (correctly) does not mark the variable as fully initialized, and so a second attempt to run the initializer will simply construct a new object on top of an already-constructed one. This is arguably correct under the standard — the first array element is not a previously-constructed object of automatic duration — but I hope it's obvious that that's a defect.

So if it was static it would just get destroyed at exit-time, and therefore should be disable-able with no_destroy. If the standard implies that we should be doing something else, then we should do that, but I can't seem to find any reference to the rule you're describing.

Like I said, this is a poorly-specified part of the standard, because at least *some* objects with static storage duration have to be destroyed when an exception is thrown (because of aggregate initialization), but the standard wants to pretend that this isn't true. I think that allowing temporary destructors to cancel initialization uniformly across all kinds of objects is the most consistent rule,

That's only true for subobjects of an enclosing aggregate before that aggregate's initialization is complete though, right? So it doesn't seem like that much of an inconsistency, just mimicking what we would be doing if an exception was thrown in, say, the body of the ctor before the object's initialization is completed.

Thu, May 2, 2:27 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D61318: [Sema] Prevent binding references with mismatching address spaces to temporaries.
Thu, May 2, 11:29 AM
rjmccall added a comment to D60454: [OpenCL] Prevent mangling kernel functions.

I think it would be more reasonable to just change getDeclLanguageLinkage to check for a kernel function.

Thu, May 2, 11:29 AM · Restricted Project
rjmccall added inline comments to D61454: [CodeGen][ObjC] Remove the leading 'l_' from ObjC symbols and make private symbols in the __DATA segment internal..
Thu, May 2, 11:23 AM · Restricted Project

Wed, May 1

rjmccall added a comment to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

It seems like the most common sense interpretation here is to just treat the initialization of G as completed at the point when the constructor finishes (this appears to be what GCC implements: https://wandbox.org/permlink/R3C9pPhoT4efAdL1).

Wed, May 1, 10:36 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D58321: Support for relative vtables.
Wed, May 1, 10:57 AM · Restricted Project, Restricted Project
rjmccall added inline comments to D61318: [Sema] Prevent binding references with mismatching address spaces to temporaries.
Wed, May 1, 10:22 AM
rjmccall accepted D61304: [OpenCL][PR41609] Deduce static data members to __global addr space.

Okay. This seems reasonable to me, then.

Wed, May 1, 10:15 AM · Restricted Project

Tue, Apr 30

rjmccall updated subscribers of D59924: [PowerPC] [Clang] Port MMX intrinsics and basic test cases to Power.

@chandlerc, I hate to do this to you, but licensing question.

Tue, Apr 30, 4:49 PM · Restricted Project
rjmccall requested changes to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.
Tue, Apr 30, 4:46 PM · Restricted Project, Restricted Project
rjmccall added a comment to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

Hmm. You know, there's another case where the destructor can be called even for a non-array: if constructing the object requires a temporary, I believe an exception thrown from that temporary's destructor is supposed to go back and destroy the variable. (This is, sadly, not entirely clear under the standard since the object is not automatic.) Now, Clang does not actually get this correct — we abort the construction, but we don't destroy the variable — but (1) we should fix that someday and (2) in the meantime, we shouldn't implement something which will be a problem when we go to fix that.

Tue, Apr 30, 4:46 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D61318: [Sema] Prevent binding references with mismatching address spaces to temporaries.
Tue, Apr 30, 4:19 PM
rjmccall added a comment to D61274: [Sema][AST] Explicit visibility for OpenCL/CUDA kernels/variables.

Okay. So it sounds like this should either be a device-only rule, with no warning in mixed-mode languages like CUDA, or we should take a different approach.

Tue, Apr 30, 1:59 PM · Restricted Project
rjmccall added a comment to D61304: [OpenCL][PR41609] Deduce static data members to __global addr space.

Presumably a similar rule would apply to thread-locals if you supported them.

Tue, Apr 30, 1:57 PM · Restricted Project
rjmccall added a comment to D61280: Variable auto-init: don't initialize aggregate padding of all aggregates.

I don't think the implication is supposed to be that padding is zero-initialized or not depending on where in the aggregate it appears, but it doesn't really matter, I don't think we're arguing about the goal of this patch.

Tue, Apr 30, 1:57 PM · Restricted Project
rjmccall added a comment to D60930: [codeview] Fix symbol names for dynamic initializers and atexit stubs.

In Swift we basically shove *everything* through our corresponding linkage-entity structure, so I'm not opposed to the basic idea.

Tue, Apr 30, 1:42 PM · Restricted Project
rjmccall added a comment to D61280: Variable auto-init: don't initialize aggregate padding of all aggregates.

IIRC, C says *members* are initialized as if they were in static storage, which might mean that their interior padding should be zeroed, but I don't think says anything about the padding in the enclosing aggregate. But I think zero-initializing padding is probably the right thing to do in general under this flag.

Tue, Apr 30, 1:34 PM · Restricted Project

Sat, Apr 27

rjmccall added a comment to D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes.

It seems to fine just forbid hidden. Again, I suspect other targets do not care because they are not using a standard dynamic loader to load the code containing kernel functions.

Sat, Apr 27, 11:10 AM · Restricted Project

Fri, Apr 26

rjmccall added a comment to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

Okay, SGTM.

Fri, Apr 26, 3:04 PM · Restricted Project, Restricted Project
rjmccall added a comment to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

I believe at least one of the goals of nodestroy is to allow the type to potentially not provide a destructor at all, so if we're going to implicitly require the destructor anyway in certain situations, we should clearly document that, and we should be aware that we may be making the attribute less useful.

Fri, Apr 26, 2:39 PM · Restricted Project, Restricted Project

Thu, Apr 25

rjmccall added a comment to D61147: [Sema][ObjC] Disable -Wunused-parameter for ObjC methods.

I do not think the ObjC version of this diagnostic is useful as it's been explained here, and I would strongly recommend just not including such a warning for the time being.

Why? It seems to me like the vast majority of methods only declared in an @implementation are used as local helpers (even if people probably should be using functions for this), where this warning would be useful. Maybe I'm missing something? Still trying to learn more about Objective-C.

Thu, Apr 25, 10:57 PM · Restricted Project, Restricted Project
rjmccall added a comment to D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array.

Are you sure these are the right semantics for nodestroy? I think there's a reasonable argument that we should not destroy previous elements of a nodestroy array just because an element constructor throws. It's basically academic for true globals because the exception will terminate the process anyway, and even for thread_locals and static locals (where I believe the exception is theoretically recoverable) it's at least arguable that we should either decline to destroy (possibly causing leaks) or simply call std::terminate.

Thu, Apr 25, 10:18 PM · Restricted Project, Restricted Project
rjmccall added a comment to D61147: [Sema][ObjC] Disable -Wunused-parameter for ObjC methods.

I do not think the ObjC version of this diagnostic is useful as it's been explained here, and I would strongly recommend just not including such a warning for the time being.

Thu, Apr 25, 10:09 PM · Restricted Project, Restricted Project
rjmccall added a comment to D61147: [Sema][ObjC] Disable -Wunused-parameter for ObjC methods.

Sorry, yes, I meant in Objective-C methods, of course, not unconditionally even in C-like code whenever Objective-C is enabled.

Thu, Apr 25, 3:14 PM · Restricted Project, Restricted Project
rjmccall added a comment to D61147: [Sema][ObjC] Disable -Wunused-parameter for ObjC methods.

Yeah, I tend to think we should just suppress this unconditionally in Objective-C.

Thu, Apr 25, 2:15 PM · Restricted Project, Restricted Project

Wed, Apr 24

rjmccall added a comment to D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes.

Yeah, that seems like a missing warning.

Wed, Apr 24, 12:58 PM · Restricted Project

Tue, Apr 23

rjmccall accepted D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes.

Yeah, that's fine.

Tue, Apr 23, 2:41 PM · Restricted Project
rjmccall added a comment to D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes.

I suspect that other OpenCL and CUDA implementations don't care at all about symbol visibility for device-side code generation, and giving kernel functions default visibility seems like the right thing to do for the (relatively few) things at the AST level that are sensitive to that, like template visibility. Would you mind reaching out to other implementors about that?

Tue, Apr 23, 2:25 PM · Restricted Project
rjmccall added a comment to D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes.

Shouldn't it be an error if the user tries to give it hidden visibility?

We effectively consider the user explicitly specifying that a symbol is e.g. a kernel to also carry with it visibility information. We don't want to require the user to redundantly specify that a kernel is not hidden, when it is never meaningful for it to be hidden.

Tue, Apr 23, 12:54 PM · Restricted Project
rjmccall added a comment to D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes.

Shouldn't it be an error if the user tries to give it hidden visibility?

Tue, Apr 23, 12:18 PM · Restricted Project
rjmccall added a comment to D60967: Move setTargetAttributes after setGVProperties in SetFunctionAttributes.

It seems reasonable to me for target hooks to run after global hooks, but can I ask why AMDGPU specifically relies on this?

Tue, Apr 23, 11:16 AM · Restricted Project

Apr 19 2019

rjmccall updated subscribers of D60748: Fix i386 struct and union parameter alignment.

I suspect Darwin also doesn't want to take this. We care very little about 32-bit Intel, and part of caring very little is not wanting to spend any effort dealing with the ramifications of ABI breaks. That would apply both to the rule in general and to the vector rule specifically. @dexonsmith, agreed?

Apr 19 2019, 11:11 AM · Restricted Project
rjmccall accepted D60850: [sema][objc] Minor refactor to OverrideSearch. NFCI..

LGTM.

Apr 19 2019, 10:18 AM · Restricted Project

Apr 18 2019

rjmccall added inline comments to D60850: [sema][objc] Minor refactor to OverrideSearch. NFCI..
Apr 18 2019, 8:02 PM · Restricted Project
rjmccall accepted D60898: MergeFunc: preserve COMDAT information when creating a thunk.

LGTM.

Apr 18 2019, 6:18 PM · Restricted Project

Apr 17 2019

rjmccall accepted D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape.

Alright, LGTM.

Apr 17 2019, 12:27 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape.
Apr 17 2019, 10:06 AM · Restricted Project, Restricted Project
rjmccall accepted D58236: Make address space conversions a bit stricter..

Alright, this seems reasonable to me.

Apr 17 2019, 8:12 AM · Restricted Project, Restricted Project

Apr 16 2019

rjmccall added inline comments to D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape.
Apr 16 2019, 8:41 PM · Restricted Project, Restricted Project

Apr 11 2019

rjmccall added a comment to D60573: [Sema] ADL: Associated namespaces for class types and enumeration types (CWG 1691).

Hmm. Does this never impact code that's just using a locally-defined type within its scope? I guess if ADL is involved, unqualified lookup must have reached the scope of the innermost namespace, and so it would be searching that namespace anyway.

In that case, I think I'm mollified that the source-compatibility risk is low and we should just unconditionally apply the new rule. LGTM.

I am not sure about what you mean. It is certainly possible to construct a piece of C++11 code which breaks with this patch.

Apr 11 2019, 10:41 PM · Restricted Project
rjmccall accepted D60573: [Sema] ADL: Associated namespaces for class types and enumeration types (CWG 1691).

Hmm. Does this never impact code that's just using a locally-defined type within its scope? I guess if ADL is involved, unqualified lookup must have reached the scope of the innermost namespace, and so it would be searching that namespace anyway.

Apr 11 2019, 3:27 PM · Restricted Project
rjmccall accepted D60548: Variable auto-init: also auto-init alloca.

LGTM.

Apr 11 2019, 3:15 PM · Restricted Project
rjmccall added inline comments to D60548: Variable auto-init: also auto-init alloca.
Apr 11 2019, 1:55 PM · Restricted Project
rjmccall added inline comments to D60548: Variable auto-init: also auto-init alloca.
Apr 11 2019, 12:56 PM · Restricted Project
rjmccall accepted D60544: Support objc_nonlazy_class attribute on Objective-C implementations.

Great, SGTM.

Apr 11 2019, 10:29 AM · Restricted Project, Restricted Project
rjmccall added a comment to D60544: Support objc_nonlazy_class attribute on Objective-C implementations.

Should we have a special has_feature test to check that this attribute is allowed on implementations? We've done that in the past when expanding the set of subjects for an attribute. But that's not necessary if we haven't made this attribute part of a public Xcode release yet, because then the attribute is just understood to always apply to implementations; I can't remember when exactly this was added and what releases it's made it into.

Apr 11 2019, 9:05 AM · Restricted Project, Restricted Project
rjmccall accepted D60542: Add support for attributes on @implementations in Objective-C.

LGTM.

Apr 11 2019, 8:59 AM · Restricted Project

Apr 10 2019

rjmccall committed rG98da442b6d77: Prospective test fix in response to r358104. (authored by rjmccall).
Prospective test fix in response to r358104.
Apr 10 2019, 8:03 PM
rjmccall committed rCRT358149: Prospective test fix in response to r358104..
Prospective test fix in response to r358104.
Apr 10 2019, 8:03 PM
rjmccall committed rL358149: Prospective test fix in response to r358104..
Prospective test fix in response to r358104.
Apr 10 2019, 8:03 PM
rjmccall committed rG827aeb461c22: Add IRGen APIs to fetch ctor/dtor helper functions for non-trivial structs. (authored by rjmccall).
Add IRGen APIs to fetch ctor/dtor helper functions for non-trivial structs.
Apr 10 2019, 12:57 PM
rjmccall committed rL358132: Add IRGen APIs to fetch ctor/dtor helper functions for non-trivial structs..
Add IRGen APIs to fetch ctor/dtor helper functions for non-trivial structs.
Apr 10 2019, 12:56 PM
rjmccall committed rC358132: Add IRGen APIs to fetch ctor/dtor helper functions for non-trivial structs..
Add IRGen APIs to fetch ctor/dtor helper functions for non-trivial structs.
Apr 10 2019, 12:56 PM
rjmccall closed D60161: Expose non-trivial struct helpers for Swift..

Committed as r358132.

Apr 10 2019, 12:56 PM · Restricted Project
rjmccall committed rG103556279fc3: Fix for different build configurations. (authored by rjmccall).
Fix for different build configurations.
Apr 10 2019, 12:11 PM
rjmccall committed rL358125: Fix for different build configurations..
Fix for different build configurations.
Apr 10 2019, 12:11 PM
rjmccall committed rC358125: Fix for different build configurations..
Fix for different build configurations.
Apr 10 2019, 12:10 PM
rjmccall committed rG7ae29f574260: Fix an off-by-one mistake in IRGen's copy-construction special cases in the… (authored by rjmccall).
Fix an off-by-one mistake in IRGen's copy-construction special cases in the…
Apr 10 2019, 11:08 AM
rjmccall closed D58016: fixes copy constructor generation of classes containing 0-length arrays followed by exactly 1 trivial field (fixes #40682).

Committed as r358115.

Apr 10 2019, 11:08 AM · Restricted Project
rjmccall committed rL358115: Fix an off-by-one mistake in IRGen's copy-construction.
Fix an off-by-one mistake in IRGen's copy-construction
Apr 10 2019, 11:05 AM
rjmccall committed rC358115: Fix an off-by-one mistake in IRGen's copy-construction.
Fix an off-by-one mistake in IRGen's copy-construction
Apr 10 2019, 11:05 AM
rjmccall added inline comments to D59988: [PR41276] Generate address space cast of 'this' for objects attributed by an address space in C++.
Apr 10 2019, 10:09 AM · Restricted Project
rjmccall committed rG8b36ac818cd5: Don't emit an unreachable return block. (authored by rjmccall).
Don't emit an unreachable return block.
Apr 10 2019, 10:02 AM
rjmccall committed rC358104: Don't emit an unreachable return block..
Don't emit an unreachable return block.
Apr 10 2019, 10:02 AM
rjmccall committed rL358104: Don't emit an unreachable return block..
Don't emit an unreachable return block.
Apr 10 2019, 10:01 AM