[cfe-dev] Continuing Adventures with Objective-C

[Eli Friedman]

On Tue, May 13, 2008 at 9:22 AM, David Chisnall <csdavec at swansea.ac.uk> wrote:
> Hi Chris,
>
> On 12 May 2008, at 20:05, Chris Lattner wrote:
>
>> This is huge.  I can't review this.  I see a whole bunch of unrelated
>> changes, please split this out into one patch per change.  This is important
>> for review, but it is also important for revision control purposes (you can
>> revert one patch without all the others).  Also, this allows patch review to
>> be done by different people which might be specialized in certain areas.
>
> Sorry about the size.  It's a bit hard to split it up, since many of the
> changes depend on others.  I'm now using svk for clang so I can split future
> diffs up more easily once my tree is a bit better sync'd with trunk.

Please include testcases with all of the patches; it makes things
easier to review, and they're necessary to commit fixes.  Please put
fixes that don't have any dependencies into separate emails; it makes
them easier to track.

A quick review of some of diffs:

vla.diff:
Nowhere near a complete implementation of vlas; better to error out
than generate bad code.  I think I sent a partial patch to this list
at one point, but I never finished it.

aggmesg.diff:
Please split this patch into its independent parts.

The VisitObjCMessageExpr implementation looks correct, although the
FIXME doesn't seem relevant, and there's no point to constructing the
RValue.

The VisitCastExpr implementation is clearly wrong; you're throwing out
the emitted value.  Also, you should add some assertions to make sure
it's a scalar to union cast.  (Also, don't resubmit this until the
Sema changes this depends on are committed.)

union.diff:
You definitely need to implement that FIXME before the patch goes in.
Also, you want to be checking for type compatibility, not pointer
equality.

types.diff:
Please split this patch into its independent parts.

The change for "case Type::ObjCQualifiedId" looks fine.

I'm a bit concerned that the implementation of ConvertReturnType might
not be appropriate to call from within ConvertNewType; that code tends
to be fragile.

super.diff:
Please make a new expession type instead of overloading PreDefinedExpr.

static.diff:
Please put the logic ObjCMethodDecl, alongside
getSynthesizedMethodSize().  (Or is that logic already elsewhere?)

ccc.diff:
Send this patch in a separate email, so that the ccc maintainer sees it.

cast.diff:
This is mixing fixes.  Please separate.

If two types are compatible, they should have the same LLVM type, I
think; is the ObjC code abusing type compatibility?  Maybe we need to
be emitting more ImplicitCasts into the AST?

-Eli