[PATCH] [RFC] PR20146 - Cannot return void with qualifiers
Richard Smith
richard at metafoo.co.uk
Tue Aug 12 15:51:30 PDT 2014
On Tue, Aug 5, 2014 at 7:28 AM, Zach Davis <zdavkeos at gmail.com> wrote:
> Thanks for the diligence,
>
> This patch follows your suggestion of using the less strict "'const'
> type qualifier on return type has no effect" on function declarations
> without definitions. Though, I wonder if we now want to include C++ on
> this less strict warning?
>
C++ already seems to have this warning:
$ echo 'const void f() {}' | ./build/bin/clang -x c++ - -Wignored-qualifiers
<stdin>:1:1: warning: 'const' type qualifier on return type has no effect
[-Wignored-qualifiers]
const void f() {}
^~~~~~
Also,
> - fixed indentation
> - added three test cases
Thanks for working on this!
+const void pr20146_4(); // expected-warning {{'const' type qualifier on
return type has no effect}}
As far as I can see, void() and const void() are not compatible types in C,
so it's not true that the 'const' has no effect. Given:
void f();
const void g();
int x = _Generic(&f, void(*)(): 0, const void(*)(): 1);
int y = _Generic(&g, void(*)(): 0, const void(*)(): 1);
x is initialized to 0, y is initialized to 1, and it is not possible to
define 'g'. I think we definitely should issue a warning on the declaration
of 'g'; we just need to word it differently.
+ // C99 6.9.1/3: The return type of a function shall be void or
+ // an object type other than array type.
+ // A return type of void cannot be qualified in C (allowed in C++).
+ if (!S.getLangOpts().CPlusPlus &&
+ RetTy->isVoidType() && RetTy.getCVRQualifiers()) {
+ unsigned DiagID;
+ if (D.isFunctionDefinition()) {
+ // The standard language only strictly applies to function
definitions.
+ DiagID = diag::ext_func_returning_qualified_void;
+ } else {
+ // For non-definition declarations, we use a less strict warning.
+ DiagID = diag::warn_qual_return_type;
+ }
+
+ diagnoseIgnoredQualifiers(S, DiagID, RetTy.getCVRQualifiers(),
+ D.getIdentifierLoc());
+ return;
+ }
The code below this point is carefully picking apart the Declarator to
figure out the location of the const/volatile/etc qualifiers. I think you
should still fall through into that code, and insert the check for which
diagnostic to emit after we've done our best to figure out the qualifier
locations.
Zach
>
> On Mon, Aug 4, 2014 at 5:55 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > Thanks, this patch looks nice. One comment and one tiny nit:
> >
> > @@ -2043,10 +2043,21 @@
> > static void diagnoseIgnoredFunctionQualifiers(Sema &S, QualType RetTy,
> > Declarator &D,
> > unsigned
> FunctionChunkIndex)
> > {
> > + // C99 6.9.1/3: The return type of a function shall be void or
> > + // an object type other than array type.
> > + // A return type of void cannot be qualified in C (allowed in C++).
> > + if (!S.getLangOpts().CPlusPlus &&
> > + RetTy->isVoidType() && RetTy.getCVRQualifiers()) {
> > + diagnoseIgnoredQualifiers(S,
> diag::ext_func_returning_qualified_void,
> > + RetTy.getCVRQualifiers(),
> > D.getIdentifierLoc());
> > + return;
> >
> > This looks like you're diagnosing (and potentially rejecting)
> cv-qualified
> > void on all function types. To my reading, C's 6.9.1/3 only applies to
> > function definitions. For whatever reason, this appears to be valid:
> >
> > const void f(); // no definition
> > const void (*p)() = NULL;
> >
> > I think we should still warn on such cases, but probably shouldn't reject
> > them in -pedantic-errors mode.
> >
> > [Such a function also can't be called. See also
> https://gcc.gnu.org/PR35210;
> > we fail to reject such constructs.]
> >
> >
> > unsigned AtomicQual = RetTy->isAtomicType() ? DeclSpec::TQ_atomic
> :
> > 0;
> > - diagnoseIgnoredQualifiers(S, RetTy.getCVRQualifiers() |
> AtomicQual,
> > - D.getIdentifierLoc());
> > + diagnoseIgnoredQualifiers(S, diag::warn_qual_return_type,
> >
> > Too much indentation here.
> >
> >
> > On Mon, Aug 4, 2014 at 9:52 AM, Zach Davis <zdavkeos at gmail.com> wrote:
> >>
> >> { including cfe-commits }
> >>
> >> Thanks for the word-smithing, and for forcing me to do it the right
> >> way by taking advantage of Sema::
> >> diagnoseIgnoredQualifiers...
> >>
> >> - Changed the wording of the warning
> >> - Parameterized Sema::diagnoseIgnoredQualifiers so we can use our new
> >> diagnostic text
> >>
> >> On Wed, Jul 16, 2014 at 5:35 PM, Richard Smith <richard at metafoo.co.uk>
> >> wrote:
> >> > It's a little confusing to put "X cannot have Y" in an ExtWarn; we
> >> > usually
> >> > phrase such things as "X has Y" or "ISO C does not allow X to have Y"
> to
> >> > make it clear that we may be allowing this despite the standard saying
> >> > otherwise. Also, the trailing %0 in the diagnostic text doesn't seem
> to
> >> > fit
> >> > into the sentence very well. Suggestion:
> >> >
> >> > ISO C does not allow %0 qualifier%plural{1:|:s} on void return type
> >> >
> >> > ... and use Sema::diagnoseIgnoredQualifiers to produce a diagnostic
> with
> >> > the
> >> > relevant qualifiers listed.
> >> >
> >> > Other than the diagnostic wording, this looks good to me, thanks.
> >> >
> >> > On Thu, Jul 10, 2014 at 7:57 AM, Zach Davis <zdavkeos at gmail.com>
> wrote:
> >> >>
> >> >> Thanks for the suggestions. In this patch:
> >> >>
> >> >> - The error is now a warning with a "return-qualified-void" group
> >> >> - The warning only applies to C code
> >> >> - Re-formatted the code
> >> >>
> >> >> The warning is currently on by default.
> >> >>
> >> >>
> >> >> On Tue, Jul 8, 2014 at 7:29 PM, Richard Smith <richard at metafoo.co.uk
> >
> >> >> wrote:
> >> >> > unsigned
> >> >> > FunctionChunkIndex)
> >> >> > {
> >> >> > +
> >> >> > + // C99 6.9.1/3: The return type of a function shall be void or
> >> >> > + // an object type other than array type.
> >> >> > + // A return type of void cannot be qualified.
> >> >> >
> >> >> >
> >> >> > Micro-nit: no blank line at the start of a function body.
> >> >> >
> >> >> > This should only apply in C; 'const void' is explicitly a valid
> >> >> > return
> >> >> > type
> >> >> > in C++ (see for instance 6.6.3/2, "a function with the return type
> cv
> >> >> > void").
> >> >> >
> >> >> > Please make this an ExtWarn rather than an error, since GCC accepts
> >> >> > it
> >> >> > by
> >> >> > default (and doesn't even diagnose it if it's on a non-definition
> >> >> > function
> >> >> > declaration), and it seems thoroughly harmless.
> >> >> >
> >> >> > On Mon, Jul 7, 2014 at 9:22 AM, Zach Davis <zdavkeos at gmail.com>
> >> >> > wrote:
> >> >> >>
> >> >> >> Thanks for the comments. I have:
> >> >> >>
> >> >> >> - Cleaned up the code
> >> >> >> - Made the warning an error
> >> >> >> - Moved the check into diagnoseIgnoredFunctionQualifiers()
> >> >> >> - Added 3 test cases to test/Sema/function.c
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >> On Thu, Jul 3, 2014 at 4:41 PM, Alp Toker <alp at nuanti.com> wrote:
> >> >> >> >
> >> >> >> > On 03/07/2014 22:08, Zach Davis wrote:
> >> >> >> >>
> >> >> >> >> As reported in bug 20146, a function cannot have a return type
> of
> >> >> >> >> 'void' with qualifiers.
> >> >> >> >>
> >> >> >> >> Clang does emit a warning that the qualifiers are ignored
> >> >> >> >> [-Wignored-qualifiers] (off by default), but according to [1]
> >> >> >> >> this
> >> >> >> >> code is non-conforming.
> >> >> >> >>
> >> >> >> >> The attached patch makes Clang issue a more specific warning
> like
> >> >> >> >> so:
> >> >> >> >>
> >> >> >> >> test3.c:8:18: warning: return type of void cannot be
> >> >> >> >> qualified
> >> >> >> >> 'volatile void'
> >> >> >> >> volatile void baz(void) { return; }
> >> >> >> >> ^
> >> >> >> >>
> >> >> >> >> [1]http://www.open-std.org/jtc1/sc22/wg14/docs/rr/dr_113.html
> >> >> >> >
> >> >> >> >
> >> >> >> > It seems fine to make this a hard error instead of a warning for
> >> >> >> > C,
> >> >> >> > and
> >> >> >> > probably C++ too. Richard?
> >> >> >> >
> >> >> >> >>
> >> >> >> >> 20146_return_qual_void.patch
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> Index: lib/Sema/SemaType.cpp
> >> >> >> >>
> >> >> >> >>
> ===================================================================
> >> >> >> >> --- lib/Sema/SemaType.cpp (revision 212275)
> >> >> >> >> +++ lib/Sema/SemaType.cpp (working copy)
> >> >> >> >> @@ -2741,6 +2741,15 @@
> >> >> >> >> D.setInvalidType(true);
> >> >> >> >> }
> >> >> >> >> + // C99 6.9.1/3: The return type of a function shall be
> >> >> >> >> void
> >> >> >> >> or
> >> >> >> >> + // an object type other than array type.
> >> >> >> >> + // A return type of void cannot be qualified.
> >> >> >> >> + if (T->isVoidType() && T.getCVRQualifiers()) {
> >> >> >> >> + unsigned diagID =
> >> >> >> >> diag::warn_func_returning_qualified_void;
> >> >> >> >
> >> >> >> >
> >> >> >> > Just pass the ID directly to Diag().
> >> >> >> >
> >> >> >> >> + S.Diag(DeclType.Loc, diagID) << T;
> >> >> >> >> + D.setInvalidType(true);
> >> >> >> >> + }
> >> >> >> >> +
> >> >> >> >
> >> >> >> >
> >> >> >> > How about placing this check with an early return at the top of
> >> >> >> > diagnoseIgnoredFunctionQualifiers()?
> >> >> >> >
> >> >> >> >> // Do not allow returning half FP value.
> >> >> >> >> // FIXME: This really should be in BuildFunctionType.
> >> >> >> >> if (T->isHalfType()) {
> >> >> >> >> Index: include/clang/Basic/DiagnosticSemaKinds.td
> >> >> >> >>
> >> >> >> >>
> ===================================================================
> >> >> >> >> --- include/clang/Basic/DiagnosticSemaKinds.td (revision
> 212275)
> >> >> >> >> +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
> >> >> >> >> @@ -4160,6 +4160,8 @@
> >> >> >> >> def err_func_returning_array_function : Error<
> >> >> >> >> "function cannot return %select{array|function}0 type %1">;
> >> >> >> >> +def warn_func_returning_qualified_void : Warning<
> >> >> >> >> + "return type of void cannot be qualified %0">;
> >> >> >> >
> >> >> >> >
> >> >> >> > (Warnings need to have a diagnostic group / -W flag, though it
> >> >> >> > doesn't
> >> >> >> > matter if you go ahead and make it an error.)
> >> >> >> >
> >> >> >> >> def err_field_declared_as_function : Error<"field %0 declared
> >> >> >> >> as a
> >> >> >> >> function">;
> >> >> >> >> def err_field_incomplete : Error<"field has incomplete type
> >> >> >> >> %0">;
> >> >> >> >> def ext_variable_sized_type_in_struct : ExtWarn<
> >> >> >> >
> >> >> >> >
> >> >> >> > Test case?
> >> >> >> >
> >> >> >> > Alp.
> >> >> >> >
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> _______________________________________________
> >> >> >> >> cfe-commits mailing list
> >> >> >> >> cfe-commits at cs.uiuc.edu
> >> >> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >> >> >> >
> >> >> >> >
> >> >> >> > --
> >> >> >> > http://www.nuanti.com
> >> >> >> > the browser experts
> >> >> >> >
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140812/c7414c71/attachment.html>
More information about the cfe-commits
mailing list