[cfe-dev] Strange return value of Sema::CheckFunctionReturnType

[Richard Smith via cfe-dev]

On 2 May 2017 1:56 pm, "Keane, Erich" <erich.keane at intel.com> wrote:





*From:* metafoo at gmail.com [mailto:metafoo at gmail.com] *On Behalf Of *Richard
Smith
*Sent:* Tuesday, May 2, 2017 1:53 PM
*To:* Keane, Erich <erich.keane at intel.com>
*Cc:* Reid Kleckner <rnk at google.com>; cfe-dev-request at lists.llvm.org;
cfe-dev at lists.llvm.org

*Subject:* Re: [cfe-dev] Strange return value of
Sema::CheckFunctionReturnType



On 2 May 2017 at 13:44, Keane, Erich via cfe-dev <cfe-dev at lists.llvm.org>
wrote:



*From:* Reid Kleckner [mailto:rnk at google.com]
*Sent:* Tuesday, May 2, 2017 1:17 PM
*To:* Keane, Erich <erich.keane at intel.com>
*Cc:* cfe-dev at lists.llvm.org; cfe-dev-request at lists.llvm.org;
chenwj.cs97g at g2.nctu.edu.tw; mats at planetcatfish.com
*Subject:* Re: [cfe-dev] Strange return value of
Sema::CheckFunctionReturnType



If this check can never fail and is cheap, we should just strengthen it to
an assertion to check that GetFullTypeForDeclarator diagnoses this for us.

*[Keane, Erich] Actually, I stand corrected…. There IS a case in our tests
where it would hit such an assertion, though it didn’t hit my breakpoint
when debugging.  I’ve never debugged ObjC before, so I must have just
messed that up.*



*That said, it IS hit in an existing test, though I’m not sure why the
‘return 0’ never caused an issue in the test.  I’ll note that it is also
missing its FixItHint, which would likely have benefit to add.  I’ll toss a
review on phabricator in a little bit that I’ll send to you all to take a
look at.*



See http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints



If you add a fixit to this error, Clang should recover as if the fixit were
applied, rather than bailing out.

*[Keane, Erich] Thanks for the clarification.  In this case, I see that the
other usage of this diagnostic includes an inseration, as does the half-FP
version, so I’d lend toward putting it here.  If I add it, is there a good
way to add this to a test?*

If you look for existing test files named fixit*, you can follow the
pattern there. Generally the ideas are to FileCheck the output of Clang in
"parseable fixits" mode, and/or to apply the fixits to a temporary file and
check that compiling the result succeeds.

On Tue, May 2, 2017 at 10:03 AM, Keane, Erich via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

I dug through this a bit.  It is pretty clearly a bug, however I was unable
to get this line to actually be hit.  It seems that
GetFullTypeForDeclarator gets called before this, and fails it first.  I'm,
not sure HOW we could possibly reproduce this, since there doesn't seem to
be a code path that would hit this before GetFullTypeForDeclarator.

Perhaps it would be valid to fix it for 'correctness' sake, and forgive its
lack of test?

Message: 6
Date: Tue, 2 May 2017 17:27:54 +0100
From: mats petersson via cfe-dev <cfe-dev at lists.llvm.org>
To: 陳韋任 <chenwj.cs97g at g2.nctu.edu.tw>
Cc: Clang Dev <cfe-dev at lists.llvm.org>
Subject: Re: [cfe-dev] Strange return value of
        Sema::CheckFunctionReturnType
Message-ID:
        <CAL-htr72Ry1Mg3t=qLeTMeK3wg9XHMoFN+NTSvEwc+Nu-_psUg at mail.gmail.com>
Content-Type: text/plain; charset="utf-8"


On 2 May 2017 at 16:08, 陳韋任 <chenwj.cs97g at g2.nctu.edu.tw> wrote:

> 2017-05-02 22:41 GMT+08:00 mats petersson <mats at planetcatfish.com>:
> > Looks like a bug to me. It should return `true`. I was going to say that
> > it's a minor bug, (where `0` and `false` are possible to convert in
> either
> > direction, but it's actually returning an incorrect result.
> >
> > A patch to fix this, and adding a test-case would probably be
> appreciated.
>
> Would be happy to. But I never use Objective-C, writing a test case would
> be
> hard to me...
>

Neither have I, but I'm pretty sure that it can't be TERRIBLY hard...

--
Mats

>
> --
> Wei-Ren Chen (陳韋任)
> Homepage: https://people.cs.nctu.edu.tw/~chenwj
>

_______________________________________________
cfe-dev mailing list
cfe-dev at lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev




_______________________________________________
cfe-dev mailing list
cfe-dev at lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170506/bfde34c4/attachment.html>