r196296 - Issue diagnostic when constructor or destructor

[jahanian]

On Dec 3, 2013, at 12:00 PM, Arthur O'Dwyer <arthur.j.odwyer at gmail.com> wrote:

> 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 (fixing my quoting :).

> 
> I don't understand what you mean by "returning a void expression is
> always an error.”

Please see Richard’s initial feedback when I fixed the IRGen.

> 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.

There is a big difference here. returning a none-void expression in C::C() is allowed under an extension warning
flag (ext_return_has_expr is ExtWarn<…>) while retuning a void expression in C::C() is always an error.
So, there is no need to fix IRGen for ARM as this construct is not allowed. Unless, of course, I mis-read Rechard’s initial
feedbeck.

- Fariborz

> 
> 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