r196296 - Issue diagnostic when constructor or destructor

[jahanian]

On Dec 3, 2013, at 2:05 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Tue, Dec 3, 2013 at 1:46 PM, jahanian <fjahanian at apple.com> wrote:
> 
> On Dec 3, 2013, at 1:19 PM, Arthur O'Dwyer <arthur.j.odwyer at gmail.com> wrote:
> 
> > Richard Smith originally wrote:
> >> This is a frontend bug, not an IRGen bug; the test case is ill-formed. "return;"
> >> can be used in a constructor or destructor, but "return <expression-with-type-void>;"
> >> cannot.
> >
> > Fariborz Jahanian wrote in response to my comments:
> >> There is a big difference here. returning a non-void expression in C::C() is allowed
> >> under an extension warning flag (ext_return_has_expr is ExtWarn<…>) while returning
> >> a void expression in C::C() is always an error.
> >
> > Returning a non-void expression in C::C() is NOT allowed, not in any
> > dialect of C++, as far as I can tell.
> > The codepath under ext_return_has_expr is actually dealing with a GNU
> > C (not C++) extension that allows "return void_expr();" (not
> > "non_void_expr") in a function returning void. Here's how to trigger
> > that diagnostic:
> >
> >    $ clang test.c -Wpedantic -c
> >    test.c:1:26: warning: void function 'bar' should not return void
> > expression [-Wpedantic]
> >    void foo(); void bar() { return foo(); }
> >                             ^      ~~~~~
> >
> > My point stands, as far as I can tell. And the reason you've had so
> > much trouble understanding this code is that it's really convoluted
> > and confusing! We should be trying to *simplify* it, not complicate it
> > by adding EVEN MORE codepaths and inconsistent behavior.
> 
> Code was simple enough to understand. I based my change on Richard’s comment
> that this should not be allowed (-Wpedantic or not).
> 
> void foo(); C::C() { return foo(); }
> 
> This cannot be consolidated with a diagnostic which allows -Wpedantic warning.
> So, another code path added to check for this condition. Feel free to change it provided
> unconditional error remains for above test case.
> 
> What happens if you build this:
> 
> int foo(); C::C() { return foo(); } 
> 
> ... for ARM, with -Wno-return-type? Does that crash too?
No. this works. Problem was trying to returning a ‘void’ expression in IR.

> 
> I was not aware that we had an extension allowing returning a (non-void) value from a function with a 'void' return type, but it appears we do. Does anyone know the history here?
> 
> Since we do have this extension (and assuming we don't intend to remove it), I agree that the return-void-expression and return-non-void-expression cases should behave the same. Within a ctor or dtor, either neither should be allowed as an extension or both should.
> 
> I think we need two changes here: 1) an IRGen fix, similar to Fariborz's original patch, but checking whether we're in a ctor/dtor rather than checking if the returned expression has type 'void', and 2) a tweak to this change, to treat the above case as an extension (to match our existing behavior).

sgtm. Consolidation of the warning/errors is now trivial. I will take a look when I get the chance.

- Thanks, Fariborz


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131203/19c0cf66/attachment.html>