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
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.
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.
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.
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
*From:* Reid Kleckner [mailto:rnk@google.com]
*Sent:* Tuesday, May 2, 2017 1:17 PM
*To:* Keane, Erich <erich.keane@intel.com>
*Cc:* cfe-dev@lists.llvm.org; cfe-dev-request@lists.llvm.org;
chenwj.cs97g@g2.nctu.edu.tw; mats@planetcatfish.com
*Subject:* Re: [cfe-dev] Strange return value of
Sema::CheckFunctionReturnTypeIf 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.
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 “Clang” CFE Internals Manual — Clang 16.0.0git documentation
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?
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
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.
