r196296 - Issue diagnostic when constructor or destructor

[Arthur O'Dwyer]

On Tue, Dec 3, 2013 at 11:27 AM, jahanian <fjahanian at apple.com> wrote:
> On Dec 3, 2013, at 10:31 AM, Arthur O'Dwyer <arthur.j.odwyer at gmail.com> wrote:
>> On Tue, Dec 3, 2013 at 9:42 AM, jahanian <fjahanian at apple.com> wrote:
>>> On Dec 3, 2013, at 9:35 AM, Jordan Rose <jordan_rose at apple.com> wrote:
>>>> On Dec 3, 2013, at 9:10 , Fariborz Jahanian <fjahanian at apple.com> wrote:
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=196296&view=rev
>>>>> Issue diagnostic when constructor or destructor return void expression.
>>>>> rdar://15366494 pr17759.
[...]
>> I don't see any significant difference between the two cases, and it
>> doesn't seem like they should have two different diagnostics issued
>> along two different (and convoluted and difficult-to-maintain)
>> codepaths.
>
> Problem I faced is that under an extension warning flag, one can return a
> non-void expression in a constructor/destructor. But returning a void
> expression is always an error.
> So, consolidating both cases into one diagnostic is not feasible. Please
> look at definition of ext_return_has_expr vs err_ctor_dtor_returns_void

(Incidentally, Fariborz, please fix your quoting. I've fixed it for you above.)

I don't understand what you mean by "returning a void expression is
always an error."
The only reason it's an error in Clang *now* is because you pushed the
change that
Jordan commented on. If you revert that change, then contrariwise,
returning a void
expression from a constructor will NEVER give a diagnostic.  I
suggested that a better
behavior (different from both the old behavior and the new behavior)
would be to make

    C::C() { return foo(); }

give a consistent diagnostic no matter what the decltype of foo is.

Incidentally, I am extremely skeptical that this patch could have 100% fixed
http://llvm.org/bugs/show_bug.cgi?id=17759
If the problem there was that the ARM backend currently has an
invariant that a ctor's AST mustn't contain "return x" for any x, then
the fix should either have been to rewrite the AST to match that
invariant (e.g. rewrite "return x" to "x; return") or else to remove
the faulty assumption from the ARM backend. Tweaking one particular
diagnostic to attempt to lock out the special case that x happens to
be (cv-qualified?) 'void' seems like the wrong approach.

–Arthur