<div dir="ltr"><div><div>Thanks, this patch looks nice. One comment and one tiny nit:</div><div><br></div><div>@@ -2043,10 +2043,21 @@</div><div> static void diagnoseIgnoredFunctionQualifiers(Sema &S, QualType RetTy,</div>
<div> Declarator &D,</div><div> unsigned FunctionChunkIndex) {</div><div>+ // C99 6.9.1/3: The return type of a function shall be void or</div>
<div>+ // an object type other than array type.</div><div>+ // A return type of void cannot be qualified in C (allowed in C++).</div><div>+ if (!S.getLangOpts().CPlusPlus &&</div><div>+ RetTy->isVoidType() && RetTy.getCVRQualifiers()) {</div>
<div>+ diagnoseIgnoredQualifiers(S, diag::ext_func_returning_qualified_void,</div><div>+ RetTy.getCVRQualifiers(), D.getIdentifierLoc());</div><div>+ return;</div></div><div><br></div><div>
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:</div>
<div><br></div><div> const void f(); // no definition</div><div> const void (*p)() = NULL;</div><div><br></div><div>I think we should still warn on such cases, but probably shouldn't reject them in -pedantic-errors mode.</div>
<div><br></div><div>[Such a function also can't be called. See also <a href="https://gcc.gnu.org/PR35210">https://gcc.gnu.org/PR35210</a>; we fail to reject such constructs.]</div><div><br></div><div><br></div><div> unsigned AtomicQual = RetTy->isAtomicType() ? DeclSpec::TQ_atomic : 0;<br>
</div><div>- diagnoseIgnoredQualifiers(S, RetTy.getCVRQualifiers() | AtomicQual,</div><div>- D.getIdentifierLoc());</div><div>+ diagnoseIgnoredQualifiers(S, diag::warn_qual_return_type,</div>
<div><br></div><div>Too much indentation here.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Aug 4, 2014 at 9:52 AM, Zach Davis <span dir="ltr"><<a href="mailto:zdavkeos@gmail.com" target="_blank">zdavkeos@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">{ including cfe-commits }<br>
<div class="im"><br>
Thanks for the word-smithing, and for forcing me to do it the right<br>
way by taking advantage of Sema::<br>
diagnoseIgnoredQualifiers...<br>
<br>
- Changed the wording of the warning<br>
- Parameterized Sema::diagnoseIgnoredQualifiers so we can use our new<br>
diagnostic text<br>
<br>
</div><div class=""><div class="h5">On Wed, Jul 16, 2014 at 5:35 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> It's a little confusing to put "X cannot have Y" in an ExtWarn; we usually<br>
> phrase such things as "X has Y" or "ISO C does not allow X to have Y" to<br>
> make it clear that we may be allowing this despite the standard saying<br>
> otherwise. Also, the trailing %0 in the diagnostic text doesn't seem to fit<br>
> into the sentence very well. Suggestion:<br>
><br>
> ISO C does not allow %0 qualifier%plural{1:|:s} on void return type<br>
><br>
> ... and use Sema::diagnoseIgnoredQualifiers to produce a diagnostic with the<br>
> relevant qualifiers listed.<br>
><br>
> Other than the diagnostic wording, this looks good to me, thanks.<br>
><br>
> On Thu, Jul 10, 2014 at 7:57 AM, Zach Davis <<a href="mailto:zdavkeos@gmail.com">zdavkeos@gmail.com</a>> wrote:<br>
>><br>
>> Thanks for the suggestions. In this patch:<br>
>><br>
>> - The error is now a warning with a "return-qualified-void" group<br>
>> - The warning only applies to C code<br>
>> - Re-formatted the code<br>
>><br>
>> The warning is currently on by default.<br>
>><br>
>><br>
>> On Tue, Jul 8, 2014 at 7:29 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> wrote:<br>
>> > unsigned<br>
>> > FunctionChunkIndex)<br>
>> > {<br>
>> > +<br>
>> > + // C99 6.9.1/3: The return type of a function shall be void or<br>
>> > + // an object type other than array type.<br>
>> > + // A return type of void cannot be qualified.<br>
>> ><br>
>> ><br>
>> > Micro-nit: no blank line at the start of a function body.<br>
>> ><br>
>> > This should only apply in C; 'const void' is explicitly a valid return<br>
>> > type<br>
>> > in C++ (see for instance 6.6.3/2, "a function with the return type cv<br>
>> > void").<br>
>> ><br>
>> > Please make this an ExtWarn rather than an error, since GCC accepts it<br>
>> > by<br>
>> > default (and doesn't even diagnose it if it's on a non-definition<br>
>> > function<br>
>> > declaration), and it seems thoroughly harmless.<br>
>> ><br>
>> > On Mon, Jul 7, 2014 at 9:22 AM, Zach Davis <<a href="mailto:zdavkeos@gmail.com">zdavkeos@gmail.com</a>> wrote:<br>
>> >><br>
>> >> Thanks for the comments. I have:<br>
>> >><br>
>> >> - Cleaned up the code<br>
>> >> - Made the warning an error<br>
>> >> - Moved the check into diagnoseIgnoredFunctionQualifiers()<br>
>> >> - Added 3 test cases to test/Sema/function.c<br>
>> >><br>
>> >><br>
>> >><br>
>> >> On Thu, Jul 3, 2014 at 4:41 PM, Alp Toker <<a href="mailto:alp@nuanti.com">alp@nuanti.com</a>> wrote:<br>
>> >> ><br>
>> >> > On 03/07/2014 22:08, Zach Davis wrote:<br>
>> >> >><br>
>> >> >> As reported in bug 20146, a function cannot have a return type of<br>
>> >> >> 'void' with qualifiers.<br>
>> >> >><br>
>> >> >> Clang does emit a warning that the qualifiers are ignored<br>
>> >> >> [-Wignored-qualifiers] (off by default), but according to [1] this<br>
>> >> >> code is non-conforming.<br>
>> >> >><br>
>> >> >> The attached patch makes Clang issue a more specific warning like<br>
>> >> >> so:<br>
>> >> >><br>
>> >> >> test3.c:8:18: warning: return type of void cannot be qualified<br>
>> >> >> 'volatile void'<br>
>> >> >> volatile void baz(void) { return; }<br>
>> >> >> ^<br>
>> >> >><br>
>> >> >> [1]<a href="http://www.open-std.org/jtc1/sc22/wg14/docs/rr/dr_113.html" target="_blank">http://www.open-std.org/jtc1/sc22/wg14/docs/rr/dr_113.html</a><br>
>> >> ><br>
>> >> ><br>
>> >> > It seems fine to make this a hard error instead of a warning for C,<br>
>> >> > and<br>
>> >> > probably C++ too. Richard?<br>
>> >> ><br>
>> >> >><br>
>> >> >> 20146_return_qual_void.patch<br>
>> >> >><br>
>> >> >><br>
>> >> >> Index: lib/Sema/SemaType.cpp<br>
>> >> >> ===================================================================<br>
>> >> >> --- lib/Sema/SemaType.cpp (revision 212275)<br>
>> >> >> +++ lib/Sema/SemaType.cpp (working copy)<br>
>> >> >> @@ -2741,6 +2741,15 @@<br>
>> >> >> D.setInvalidType(true);<br>
>> >> >> }<br>
>> >> >> + // C99 6.9.1/3: The return type of a function shall be void<br>
>> >> >> or<br>
>> >> >> + // an object type other than array type.<br>
>> >> >> + // A return type of void cannot be qualified.<br>
>> >> >> + if (T->isVoidType() && T.getCVRQualifiers()) {<br>
>> >> >> + unsigned diagID =<br>
>> >> >> diag::warn_func_returning_qualified_void;<br>
>> >> ><br>
>> >> ><br>
>> >> > Just pass the ID directly to Diag().<br>
>> >> ><br>
>> >> >> + S.Diag(DeclType.Loc, diagID) << T;<br>
>> >> >> + D.setInvalidType(true);<br>
>> >> >> + }<br>
>> >> >> +<br>
>> >> ><br>
>> >> ><br>
>> >> > How about placing this check with an early return at the top of<br>
>> >> > diagnoseIgnoredFunctionQualifiers()?<br>
>> >> ><br>
>> >> >> // Do not allow returning half FP value.<br>
>> >> >> // FIXME: This really should be in BuildFunctionType.<br>
>> >> >> if (T->isHalfType()) {<br>
>> >> >> Index: include/clang/Basic/DiagnosticSemaKinds.td<br>
>> >> >> ===================================================================<br>
>> >> >> --- include/clang/Basic/DiagnosticSemaKinds.td (revision 212275)<br>
>> >> >> +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)<br>
>> >> >> @@ -4160,6 +4160,8 @@<br>
>> >> >> def err_func_returning_array_function : Error<<br>
>> >> >> "function cannot return %select{array|function}0 type %1">;<br>
>> >> >> +def warn_func_returning_qualified_void : Warning<<br>
>> >> >> + "return type of void cannot be qualified %0">;<br>
>> >> ><br>
>> >> ><br>
>> >> > (Warnings need to have a diagnostic group / -W flag, though it<br>
>> >> > doesn't<br>
>> >> > matter if you go ahead and make it an error.)<br>
>> >> ><br>
>> >> >> def err_field_declared_as_function : Error<"field %0 declared as a<br>
>> >> >> function">;<br>
>> >> >> def err_field_incomplete : Error<"field has incomplete type %0">;<br>
>> >> >> def ext_variable_sized_type_in_struct : ExtWarn<<br>
>> >> ><br>
>> >> ><br>
>> >> > Test case?<br>
>> >> ><br>
>> >> > Alp.<br>
>> >> ><br>
>> >> >><br>
>> >> >><br>
>> >> >> _______________________________________________<br>
>> >> >> cfe-commits mailing list<br>
>> >> >> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> >> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>> >> ><br>
>> >> ><br>
>> >> > --<br>
>> >> > <a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
>> >> > the browser experts<br>
>> >> ><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>