- User Since
- Jan 18 2013, 11:30 AM (266 w, 12 h)
Thu, Feb 22
Wed, Feb 21
I hate inalloca so much. Okay.
Mon, Feb 19
Discourse nitpick: I would encourage you to just use the ordinary phrase "null pointer", or just "null", when referring to a pointer value that happens to be null and to reserve "nullptr" for *statically* null pointers, especially the nullptr expression.
I'm fine with that plan. LGTM.
Fri, Feb 16
Thu, Feb 15
That's not really okay; there are some places that make guarantees about call-argument ordering, and in general we want that to be decided at a higher level, rather than having a particular order forced in corner cases just to satisfy a lower-level implementation requirement.
Wed, Feb 14
Tue, Feb 13
Mon, Feb 12
Sun, Feb 11
No, this LGTM.
Fri, Feb 9
Hmm. I think if you're going to push for this, you need to put more time into making sure that the diagnostics are good, and most them seem pretty questionable.
Thu, Feb 8
No worries. LGTM!
Oh. It's not a good idea to try to match exact local IR names like this for two reasons:
- First, many IR names are different in builds with and without assertions.
- Second, it's pretty susceptible to innocuous changes in output.
I'm somewhat surprised LLVM doesn't already canonicalize this, but ok.
Thanks, that looks a lot better. One minor piece of feedback, but otherwise LGTM.
Okay, thanks. LGTM.
Wed, Feb 7
I don't understand why your description of this patch mentions the void* placement new operator. There's no cookie to poison for that operator.
Tue, Feb 6
Fri, Feb 2
No, really, in CreateBuiltinArraySubscriptExpr and LookupMemberExpr, in the clauses where they handle vector types, you just need to propagate qualifiers from the base type like is done in BuildFieldReferenceExpr. There's even a FIXME about it in the former.
If you just want a better diagnostic, there's quite a bit of machinery already set up to give more specific errors about why such-and-such l-value isn't modifiable. There may even be vector-specific diagnostics for that already, just triggered from the wrong place in the code.
Thu, Feb 1
That's still just const-propagation. The const qualifier on the pointee type of this should propagate to the l-value resulting from the member access, and the vector-specific projection logic should continue to propagate const to the type of the element l-value.
Wed, Jan 31
No, I mean things like void foo(__attribute__((swiftcall)) void (*fnptr)());.
Is demangling "correctly" really a more important goal here than not spuriously failing when presented with a swiftcall function type in a non-top-level position? I don't know that there was anything wrong with the previous patch's approach to this if we're just going to punt on handling it properly because MS happens to not have an official mangling for it.
Tue, Jan 30
Oh, that makes much more sense, thanks.
Mon, Jan 29
This seems reasonable, but I don't generally review LLVM patches.
Sat, Jan 27
It's not just that functions can't be overloaded on the parameter-variable type qualifier — it's not part of the function type at all, just like making a parameter 'const int' instead of 'int' is not part of the function type. I understand that MSVC mangles some things that really shouldn't be mangled, but I would greatly prefer if you could make an exception for this.
Fri, Jan 26
Thu, Jan 25
Jan 24 2018
The basic idea here seems fine to me; I'll leave David to review the details.
Okay, that works, thanks! LGTM.
Jan 22 2018
Well, my point is that the example in the linked bug is asking for 486 code-generation, which is apparently unsupported by LLVM.
This is definitely something that the personality function should handle. If we want to do heroic things in the absence of personality function support, we can, but the code should at least be written to be conditional on personality support.
Thank you. Maybe there should be an overload of EmitAggregateCopy that takes LValues? A lot of these cases start with an LValue on at least one side, and there are already some convenience functions to turn an Address into a naturally-typed LValue.
Looks great to me! Thanks for taking this on, it's a pretty major improvement for users.
Jan 12 2018
Thanks. LGTM, but you should wait for Eli's sign-off, too.
Thanks, that looks great. I appreciate you doing this.
Jan 9 2018
Jan 8 2018
Thanks, looks good to me.
Yes, I think that's fine.
Jan 6 2018
Jan 5 2018
I'll trust Richard on the tricky Sema/AST bits. The functionality of the patch looks basically acceptable to me, although I'm still not thrilled about the idea of actually removing the attribute from the AST rather than just letting it not have effect. But we could clean that up later if it's significantly simpler to do it this way.
Jan 4 2018
Jan 3 2018
Jan 2 2018
Aren't these always loads and stores from allocas? I would expect TBAA to be useless here because it's always strictly less informative than basic-AA, which knows that the access doesn't alias with anything.
You're sure you just want a single TBAA tag on memcpy's? I would think that it might be useful to encode separate path information for the two operands.
I think at least one of the MIPS ABIs does the same thing with varargs as AArch64 on Darwin.
I'm glad to hear that progress is finally happening on this.
Dec 21 2017
Interesting. Okay, LGTM.
That's great, thanks. LGTM.
Dec 20 2017
You can pass multiple -check-prefix arguments to FileCheck and it'll match all of them. You can use that to make your test change simpler: make the existing RUN check for both PATH and OLD-PATH and the new RUN check for both PATH and NEW-PATH, then change all the existing metadata matches to OLD-PATH.
Okay, I think that's reasonable enough. LGTM.
Dec 19 2017
Rewriting some of the most basic tests would be fine. Please either use new FileCheck lines or clone the existing tests, since we don't really know how long this transition will last.
Dec 18 2017
Dec 15 2017
I accidentally hit 'send' too early on my review, so here's part two. I still haven't fully reviewed the new IRGen file.
You should add a has_feature check for this (has_feature(objc_arc_fields)?), as well as documentation. The documentation would go in the ARC spec.
LGTM outside of a comment request; please feel free to commit when you'd made that change.