Quantcast

Strange return value of Sema::CheckFunctionReturnType

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Strange return value of Sema::CheckFunctionReturnType

Lang Hames via cfe-dev
Hi All,

  While reading Sema::CheckFunctionReturnType in SemaType.cpp [1], I
find something strange.
From the context, the return value of CheckFunctionReturnType indicate
something is invalid.
If the check fail, CheckFunctionReturnType returns true.

The other two checks in CheckFunctionReturnType return true as I
expected. But the following
check return 0, which should be false.

    if (T->isObjCObjectType()) {
      Diag(Loc, diag::err_object_cannot_be_passed_returned_by_value) << 0 << T;
      return 0;
    }

Is it a bug? Or I am reading the code wrong?

Thanks.

[1] https://clang.llvm.org/doxygen/SemaType_8cpp_source.html

Regards,
chenwj

--
Wei-Ren Chen (陳韋任)
Homepage: https://people.cs.nctu.edu.tw/~chenwj
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Strange return value of Sema::CheckFunctionReturnType

Lang Hames via cfe-dev
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.

--
Mats

On 2 May 2017 at 15:26, 陳韋任 via cfe-dev <[hidden email]> wrote:
Hi All,

  While reading Sema::CheckFunctionReturnType in SemaType.cpp [1], I
find something strange.
From the context, the return value of CheckFunctionReturnType indicate
something is invalid.
If the check fail, CheckFunctionReturnType returns true.

The other two checks in CheckFunctionReturnType return true as I
expected. But the following
check return 0, which should be false.

    if (T->isObjCObjectType()) {
      Diag(Loc, diag::err_object_cannot_be_passed_returned_by_value) << 0 << T;
      return 0;
    }

Is it a bug? Or I am reading the code wrong?

Thanks.

[1] https://clang.llvm.org/doxygen/SemaType_8cpp_source.html

Regards,
chenwj

--
Wei-Ren Chen (陳韋任)
Homepage: https://people.cs.nctu.edu.tw/~chenwj
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Strange return value of Sema::CheckFunctionReturnType

Lang Hames via cfe-dev
2017-05-02 22:41 GMT+08:00 mats petersson <[hidden email]>:
> 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...

--
Wei-Ren Chen (陳韋任)
Homepage: https://people.cs.nctu.edu.tw/~chenwj
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Strange return value of Sema::CheckFunctionReturnType

Lang Hames via cfe-dev


On 2 May 2017 at 16:08, 陳韋任 <[hidden email]> wrote:
2017-05-02 22:41 GMT+08:00 mats petersson <[hidden email]>:
> 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Strange return value of Sema::CheckFunctionReturnType

Lang Hames via cfe-dev
In reply to this post by Lang Hames via cfe-dev
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 <[hidden email]>
To: 陳韋任 <[hidden email]>
Cc: Clang Dev <[hidden email]>
Subject: Re: [cfe-dev] Strange return value of
        Sema::CheckFunctionReturnType
Message-ID:
        <CAL-htr72Ry1Mg3t=[hidden email]>
Content-Type: text/plain; charset="utf-8"

On 2 May 2017 at 16:08, 陳韋任 <[hidden email]> wrote:

> 2017-05-02 22:41 GMT+08:00 mats petersson <[hidden email]>:
> > 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Strange return value of Sema::CheckFunctionReturnType

Lang Hames via cfe-dev
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.

On Tue, May 2, 2017 at 10:03 AM, Keane, Erich via cfe-dev <[hidden email]> 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 <[hidden email]>
To: 陳韋任 <[hidden email]>
Cc: Clang Dev <[hidden email]>
Subject: Re: [cfe-dev] Strange return value of
        Sema::CheckFunctionReturnType
Message-ID:
        <CAL-htr72Ry1Mg3t=[hidden email]>
Content-Type: text/plain; charset="utf-8"

On 2 May 2017 at 16:08, 陳韋任 <[hidden email]> wrote:

> 2017-05-02 22:41 GMT+08:00 mats petersson <[hidden email]>:
> > 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Strange return value of Sema::CheckFunctionReturnType

Lang Hames via cfe-dev

That seems fair to me.  Chenwj: I’ll let you fix this one if you’d like, otherwise I’ve got it open in a terminal.

 

From: Reid Kleckner [mailto:[hidden email]]
Sent: Tuesday, May 2, 2017 1:17 PM
To: Keane, Erich <[hidden email]>
Cc: [hidden email]; [hidden email]; [hidden email]; [hidden email]
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.

 

On Tue, May 2, 2017 at 10:03 AM, Keane, Erich via cfe-dev <[hidden email]> 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 <[hidden email]>
To: 陳韋任 <[hidden email]>
Cc: Clang Dev <[hidden email]>
Subject: Re: [cfe-dev] Strange return value of
        Sema::CheckFunctionReturnType
Message-ID:
        <CAL-htr72Ry1Mg3t=[hidden email]>
Content-Type: text/plain; charset="utf-8"


On 2 May 2017 at 16:08, 陳韋任 <[hidden email]> wrote:

> 2017-05-02 22:41 GMT+08:00 mats petersson <[hidden email]>:
> > 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

 


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Strange return value of Sema::CheckFunctionReturnType

Lang Hames via cfe-dev
In reply to this post by Lang Hames via cfe-dev

 

From: Reid Kleckner [mailto:[hidden email]]
Sent: Tuesday, May 2, 2017 1:17 PM
To: Keane, Erich <[hidden email]>
Cc: [hidden email]; [hidden email]; [hidden email]; [hidden email]
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.

 

On Tue, May 2, 2017 at 10:03 AM, Keane, Erich via cfe-dev <[hidden email]> 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 <[hidden email]>
To: 陳韋任 <[hidden email]>
Cc: Clang Dev <[hidden email]>
Subject: Re: [cfe-dev] Strange return value of
        Sema::CheckFunctionReturnType
Message-ID:
        <CAL-htr72Ry1Mg3t=[hidden email]>
Content-Type: text/plain; charset="utf-8"


On 2 May 2017 at 16:08, 陳韋任 <[hidden email]> wrote:

> 2017-05-02 22:41 GMT+08:00 mats petersson <[hidden email]>:
> > 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

 


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Strange return value of Sema::CheckFunctionReturnType

Lang Hames via cfe-dev
On 2 May 2017 at 13:44, Keane, Erich via cfe-dev <[hidden email]> wrote:

 

From: Reid Kleckner [mailto:[hidden email]]
Sent: Tuesday, May 2, 2017 1:17 PM
To: Keane, Erich <[hidden email]>
Cc: [hidden email]; [hidden email]; [hidden email]; [hidden email]
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.



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

On Tue, May 2, 2017 at 10:03 AM, Keane, Erich via cfe-dev <[hidden email]> 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 <[hidden email]>
To: 陳韋任 <[hidden email]>
Cc: Clang Dev <[hidden email]>
Subject: Re: [cfe-dev] Strange return value of
        Sema::CheckFunctionReturnType
Message-ID:
        <CAL-htr72Ry1Mg3t=[hidden email]>
Content-Type: text/plain; charset="utf-8"


On 2 May 2017 at 16:08, 陳韋任 <[hidden email]> wrote:

> 2017-05-02 22:41 GMT+08:00 mats petersson <[hidden email]>:
> > 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

 


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Strange return value of Sema::CheckFunctionReturnType

Lang Hames via cfe-dev

 

 

From: [hidden email] [mailto:[hidden email]] On Behalf Of Richard Smith
Sent: Tuesday, May 2, 2017 1:53 PM
To: Keane, Erich <[hidden email]>
Cc: Reid Kleckner <[hidden email]>; [hidden email]; [hidden email]
Subject: Re: [cfe-dev] Strange return value of Sema::CheckFunctionReturnType

 

On 2 May 2017 at 13:44, Keane, Erich via cfe-dev <[hidden email]> wrote:

 

From: Reid Kleckner [mailto:[hidden email]]
Sent: Tuesday, May 2, 2017 1:17 PM
To: Keane, Erich <[hidden email]>
Cc: [hidden email]; [hidden email]; [hidden email]; [hidden email]
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.

 

 

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?

 

On Tue, May 2, 2017 at 10:03 AM, Keane, Erich via cfe-dev <[hidden email]> 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 <[hidden email]>
To: 陳韋任 <[hidden email]>
Cc: Clang Dev <[hidden email]>
Subject: Re: [cfe-dev] Strange return value of
        Sema::CheckFunctionReturnType
Message-ID:
        <CAL-htr72Ry1Mg3t=[hidden email]>
Content-Type: text/plain; charset="utf-8"


On 2 May 2017 at 16:08, 陳韋任 <[hidden email]> wrote:

> 2017-05-02 22:41 GMT+08:00 mats petersson <[hidden email]>:
> > 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

 


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

 


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Strange return value of Sema::CheckFunctionReturnType

Lang Hames via cfe-dev
In reply to this post by Lang Hames via cfe-dev


2017-05-03 4:19 GMT+08:00 Keane, Erich <[hidden email]>:

That seems fair to me.  Chenwj: I’ll let you fix this one if you’d like, otherwise I’ve got it open in a terminal.


​Go ahead, please. I'll follow up on https://reviews.llvm.org/D32759. :-)

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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Strange return value of Sema::CheckFunctionReturnType

Lang Hames via cfe-dev
In reply to this post by Lang Hames via cfe-dev
Hi Eli,

  I see you edited the code in svn revision 184006. Do you still remember why we return zero rather than true like other checks in `Sema::CheckFunctionReturnType`? Erich has a patch on https://reviews.llvm.org/D32759 waiting for review.

  Thanks.

Regards,
chenwj

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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Strange return value of Sema::CheckFunctionReturnType

Lang Hames via cfe-dev
In reply to this post by Lang Hames via cfe-dev
On 2 May 2017 1:56 pm, "Keane, Erich" <[hidden email]> wrote:

 

 

From: [hidden email] [mailto:[hidden email]] On Behalf Of Richard Smith
Sent: Tuesday, May 2, 2017 1:53 PM
To: Keane, Erich <[hidden email]>
Cc: Reid Kleckner <[hidden email]>; [hidden email]; [hidden email]


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

 

On 2 May 2017 at 13:44, Keane, Erich via cfe-dev <[hidden email]> wrote:

 

From: Reid Kleckner [mailto:[hidden email]]
Sent: Tuesday, May 2, 2017 1:17 PM
To: Keane, Erich <[hidden email]>
Cc: [hidden email]; [hidden email]; [hidden email]; [hidden email]
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.

 

 

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 <[hidden email]> 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 <[hidden email]>
To: 陳韋任 <[hidden email]>
Cc: Clang Dev <[hidden email]>
Subject: Re: [cfe-dev] Strange return value of
        Sema::CheckFunctionReturnType
Message-ID:
        <CAL-htr72Ry1Mg3t=[hidden email]>
Content-Type: text/plain; charset="utf-8"


On 2 May 2017 at 16:08, 陳韋任 <[hidden email]> wrote:

> 2017-05-02 22:41 GMT+08:00 mats petersson <[hidden email]>:
> > 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

 


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

 



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Loading...