[patch] mergefunc struct result

Duncan P. N. Exon Smith dexonsmith at apple.com
Wed Apr 30 08:21:13 PDT 2014


On 2014 Apr 30, at 00:10, Carlo Kok <ck at remobjects.com> wrote:

> On Sun, 27 Apr 2014 23:33:30 +0200, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> I applied your patch and ran the test locally, and it seems to work.
>> 
>> I think you should add `assert(!DestTy->isStructTy())` after the `if`
>> statement.
>> 
>> Otherwise, this is looking good.  Just a few oddities in the test to
>> clean up.
>> 
>> 1. Remove the semi-colon in `mergefunc-struct-return.ll` after your
>>    declaration of `@noop`.  In IR, semi-colons start comments, but
>>    you don't have a comment there.
>> 
>> 2. In your testcase, I'm seeing `^M` at the end of every line.
>>    Please remove the linefeeds.  The testcase is missing a newline
>>    at the end of the file, too, but I'm guessing it has a linefeed.
>>    Make sure it has a newline (but don't add an empty line...).
>>    There's also a trailing space character in `@fn1` (after the
>>    closing `}`).  All of these issues would be detected by
>>    `git diff --check`.
> 
> Sort of; the ^M's were really just Windows line enters; Fixed.
> 
>> 
>> 3. Add a `CHECK-LABEL: @fn1(`.
>> 
>> 4. Add a comment at the top of the file (after the `RUN` line) to
>>    describe what the test is about.
>> 
> 
> Done, new version attached.

LGTM.

I just noticed another couple of extra semi-colons: on your calls
of @noop().  Please remove those too before commit.



More information about the llvm-commits mailing list