[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