rsmith added a comment.
In D119778#3327161 <https://reviews.llvm.org/D119778#3327161>, @Quuxplusone
wrote:
> Apply @rsmith's suggested approach.
Thanks, the structure of the change looks good to me.
Looking through the test changes, I'm a bit torn by the value proposition of
this note. Some of the errors were already pointing at the return type itself,
and others were pointing at the return statement. I can see value in making
sure that we mention both when diagnosing return type deduction errors, but
pointing out the return type twice and never mentioning the return statement
seems a bit noisy. I don't think I have any great suggestions here, though;
there's not really a great way we can determine which of the two a diagnostic
is pointing at so we can point the note at the other one.
The cases that I found in the tests that seem particularly bad today:
template<typename T> concept Never = false;
void g();
// error: cannot form a reference to 'void'
auto &f() {
// ...
return g();
}
// error: deduced type 'void' does not satisfy 'Never'
Never auto h() {
// ...
return g();
}
... would both benefit from a note saying "return type deduced from returned
expression of type 'void' here" or something like that, and aren't really
helped much by this change. But I think noting *both* the return statement and
the return type would be too much (then we guarantee we always have at least
one noisy note). :(
What do you think? This seems on balance to be near the edge between a useful
note and a noisy note, though in many of the specific cases it's pretty clearly
on one side or the other.
>> I don't see any obvious way that this patch could be responsible for that
>> failure, unless it's something like a pre-existing use of uninitialized
>> memory or a use-after-free and this patch is just changing the happenstance
>> behavior.
>
> Well, it might more likely be the fault of D119184
> <https://reviews.llvm.org/D119184> rather than this one per se. D119184
> <https://reviews.llvm.org/D119184> is doing things with `Context.VoidTy` that
> weren't there before; could any of that new code be the culprit?
That seems more likely, but nothing in that patch is jumping out at me.
Especially given that it only affects the behavior of return type deduction in
a function with either no return statements or only `return;`, it's pretty
weird that libc++ would even exercise the patch.
================
Comment at: clang/test/SemaCXX/deduced-return-void.cpp:153-154
};
auto l5 = []() -> auto& { // expected-error {{cannot form a reference to
'void'}}
+ // expected-note@-1 {{deducing return type for
'operator()'}}
return i;
----------------
This seems like an unfortunate case: both the error and the note point at the
return type and neither points at the return statement.
================
Comment at: clang/test/SemaTemplate/concepts.cpp:177-178
C auto f1() { return void(); } // expected-error {{deduced type
'void' does not satisfy 'C'}}
+ // expected-note@-1 {{deducing
return type for 'f1'}}
C auto f2() { return; } // expected-error {{deduced type
'void' does not satisfy 'C'}}
----------------
This is an unfortunate case: the existing error message already points at the
`C auto` constraint, so we now have an error and a note both pointing at the
return type, and nothing pointing to the `return` statement itself.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119778/new/
https://reviews.llvm.org/D119778
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
