[PATCH] [RFC] PR20146 - Cannot return void with qualifiers
Zach Davis
zdavkeos at gmail.com
Tue Aug 5 07:28:52 PDT 2014
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?
Also,
- fixed indentation
- added three test cases
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 --------------
A non-text attachment was scrubbed...
Name: 20146_return_qual_void_5.patch
Type: text/x-patch
Size: 5583 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140805/1ef72e3d/attachment.bin>
More information about the cfe-commits
mailing list