<div dir="ltr">On Mon, Apr 29, 2013 at 10:31 AM, Serge Pavlov <span dir="ltr"><<a href="mailto:sepavloff@gmail.com" target="_blank">sepavloff@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<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"><div dir="ltr"><div>Hi Richard,<br><br></div><div class="gmail_extra">
<br><div class="gmail_extra">2013/4/29 Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span><br>
<div>[...] <br></div><div class="im"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr">The problem is that clang in C++ mode accepts the code:<br>
<div class="gmail_extra">
<div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div> int foo(xxx);<br>
Clang intentionally accepts this code due to a check in Parser::ParseImplicitInt, which appeared in r156854.<br>
The comment in the inserted code states that MS supports implicit int in C++ mode, however it looks like none of VS2005, VS2008, VS2010 or VS2012 does it. So removing the check for MS extension solves the problem.</div></div>
</blockquote><div><br></div></div><div>If it is indeed the case that MSVC does not allow implicit int in C++, then we should absolutely remove that "extension". However, someone presumably added it for a reason, so I'd like to be sure that we've checked this thoroughly before proceeding. Does MSVC allow implicit int in any other contexts? For instance...</div>
</div></div></div></blockquote><div> <br></div></div><div>MSVC doesn't allow implicit int in any context if in C++ mode, details are in bugzilla.<br><br></div><div class="im"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div> </div></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr">
<div class="gmail_extra"><div class="gmail_quote">
<div>const n = 0; // ok?</div><div>static f() { // ok?</div><div> extern m; // ok?</div><div> return m;</div><div>}</div></div></div></div></blockquote><div> </div></div><div>None of these cases are accepted by MSVC. <br>
</div><div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>If MSVC does allow these, then the fix is different: the decl-specifier-seq (or, in C, the declaration-specifiers) for a parameter cannot be empty, so 'int foo(xxx);' would not have implicit int applied, whereas 'int foo(const xxx);' would, and we should make the parser handle that correctly.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>
Another problem - the same code compiled in C mode produces an error, while both GCC and MSC accept it. To fix it the message err_ident_list_in_fn_declaration was converted into warning.<br></div></div></blockquote><div>
<br></div></div><div>Have you checked whether they treat it as an implicit int, or whether they treat it as an (ignored, presumably) identifier list?</div></div></div></div></blockquote><div><br></div></div><div>They are ignored. For instance, both MSVC and GCC successfully compile the following code:<br>
<br>void abc(xxx);<br>void abc(int x, char*y) {}<br> <br></div><div class="im"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div> Also, do you actually have code which relies upon this extension? If not, let's not add it gratuitously.</div></div></div></div></blockquote><div> </div></div><div>I know nothing about such, the intent was to make behavior more compatible. Probably it doesn't worth implementation.<br>
<br></div><div class="im"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div>Please split this into its two constituent changes (removing implicit int in microsoft mode, and accepting an identifier-list in a non-defining function declaration). They're basically unrelated, and make more sense to review separately.</div>
</div></div></div></blockquote><div> </div></div><div>OK. This patch only removes implicit int in MS-compatibility mode for C++. Fix to accepting an identifier-list in a non-defining function declaration is dropped.</div>
</div></div></div></blockquote><div><br></div><div style>Thanks, committed as r180822.<br></div><div> </div><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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_extra"><div><span style="color:rgb(80,0,80)"> </span></div><div class="im"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div>Files:<br> include/clang/Basic/DiagnosticSemaKinds.td<br>
lib/Parse/ParseDecl.cpp<br> lib/Sema/SemaType.cpp<br> test/Sema/MicrosoftCompatibility.cpp<br> test/Sema/alloc_size.c<br> test/Sema/function.c<br> test/Sema/invalid-decl.c<br><br>-----------------------------------------------------------------------------------------------------<br>
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td<br>index 1461716..166dbab 100644<br>--- a/include/clang/Basic/DiagnosticSemaKinds.td<br>+++ b/include/clang/Basic/DiagnosticSemaKinds.td<br>
@@ -2314,8 +2314,9 @@ def err_void_only_param : Error<<br> "'void' must be the first and only parameter if specified">;<br> def err_void_param_qualified : Error<<br> "'void' as parameter must not have type qualifiers">;<br>
-def err_ident_list_in_fn_declaration : Error<<br>- "a parameter list without types is only allowed in a function definition">;<br>+def warn_ident_list_in_fn_declaration : Warning<<br>+ "a parameter list without types is only allowed in a function definition">,<br>
+ InGroup<C99Compat>;<br></div></div></blockquote><div><br></div></div><div>This should be an ExtWarn, not a Warning, since this is a required diagnostic per the various C language standards. Also, C99Compat seems wrong.</div>
</div></div></div></blockquote><div><br></div></div><div>Thank you for the explanation. <br><br></div><div class="im"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div dir="ltr">
<div class="gmail_extra"><div class="gmail_quote"><div>
<div> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div> def ext_param_not_declared : Extension<<br> "parameter %0 was not declared, defaulting to type 'int'">;<br> def err_param_typedef_of_void : Error<<br>diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp<br>
index d786ce2..2f0c1a3 100644<br>--- a/lib/Parse/ParseDecl.cpp<br>+++ b/lib/Parse/ParseDecl.cpp<br>@@ -2038,10 +2038,9 @@ bool Parser::ParseImplicitInt(DeclSpec &DS, CXXScopeSpec *SS,<br> // error, do lookahead to try to do better recovery. This never applies<br>
// within a type specifier. Outside of C++, we allow this even if the<br> // language doesn't "officially" support implicit int -- we support<br>- // implicit int as an extension in C99 and C11. Allegedly, MS also<br>
- // supports implicit int in C++ mode.<br>+ // implicit int as an extension in C99 and C11.<br> if (DSC != DSC_type_specifier && DSC != DSC_trailing &&<br>- (!getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt) &&<br>
+ !getLangOpts().CPlusPlus &&<br></div></div></blockquote><div><br></div></div><div>There is a matching check in lib/Sema/DeclSpec.cpp, and possibly elsewhere. If we're not enabling implicit int in -fms-extensions mode, we need to do that consistently throughout the compiler.</div>
</div></div></div></blockquote><div><br></div></div><div>Indeed, SemaType.cpp also contains similar check. <br></div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div>
isValidAfterIdentifierInDeclarator(NextToken())) {<br> // If this token is valid for implicit int, e.g. "static x = 4", then<br>
// we just avoid eating the identifier, so it will be parsed as the<br>
diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp<br>index 8bf5143..243b772 100644<br>--- a/lib/Sema/SemaType.cpp<br>+++ b/lib/Sema/SemaType.cpp<br>@@ -2742,7 +2742,7 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,<br>
if (FTI.NumArgs && FTI.ArgInfo[0].Param == 0) {<br> // C99 6.7.5.3p3: Reject int(x,y,z) when it's not a function<br> // definition.<br>- S.Diag(FTI.ArgInfo[0].IdentLoc, diag::err_ident_list_in_fn_declaration);<br>
+ S.Diag(FTI.ArgInfo[0].IdentLoc, diag::warn_ident_list_in_fn_declaration);<br> D.setInvalidType(true);<br></div></div></blockquote><div><br></div></div><div>If you're not issuing an error, you must build a correct AST -- you can't set things invalid.</div>
<div> </div></div></div></div></blockquote></div><div>My fault... <br>[...]<br><br><br></div>Updated patch:<br><br></div></div><div class="gmail_extra">Files:<br> lib/Parse/ParseDecl.cpp<br> lib/Sema/DeclSpec.cpp<br> lib/Sema/SemaType.cpp<br>
test/Rewriter/<a href="http://rewrite-byref-in-nested-blocks.mm" target="_blank">rewrite-byref-in-nested-blocks.mm</a><br> test/Sema/MicrosoftCompatibility.cpp<div class="im"><br><br>diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp<br>
index d786ce2..2f0c1a3 100644<br>
--- a/lib/Parse/ParseDecl.cpp<br>+++ b/lib/Parse/ParseDecl.cpp<br>@@ -2038,10 +2038,9 @@ bool Parser::ParseImplicitInt(DeclSpec &DS, CXXScopeSpec *SS,<br> // error, do lookahead to try to do better recovery. This never applies<br>
// within a type specifier. Outside of C++, we allow this even if the<br> // language doesn't "officially" support implicit int -- we support<br>- // implicit int as an extension in C99 and C11. Allegedly, MS also<br>
- // supports implicit int in C++ mode.<br>+ // implicit int as an extension in C99 and C11.<br> if (DSC != DSC_type_specifier && DSC != DSC_trailing &&<br>- (!getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt) &&<br>
+ !getLangOpts().CPlusPlus &&<br></div><div class="im"> isValidAfterIdentifierInDeclarator(NextToken())) {<br> // If this token is valid for implicit int, e.g. "static x = 4", then<br> // we just avoid eating the identifier, so it will be parsed as the<br>
</div>
diff --git a/lib/Sema/DeclSpec.cpp b/lib/Sema/DeclSpec.cpp<br>index 124f50c..3b3ab2c 100644<br>--- a/lib/Sema/DeclSpec.cpp<br>+++ b/lib/Sema/DeclSpec.cpp<br>@@ -1003,8 +1003,7 @@ void DeclSpec::Finish(DiagnosticsEngine &D, Preprocessor &PP) {<br>
// the type specifier is not optional, but we got 'auto' as a storage<br> // class specifier, then assume this is an attempt to use C++0x's 'auto'<br> // type specifier.<br>- // FIXME: Does Microsoft really support implicit int in C++?<br>
- if (PP.getLangOpts().CPlusPlus && !PP.getLangOpts().MicrosoftExt &&<br>+ if (PP.getLangOpts().CPlusPlus &&<br> TypeSpecType == TST_unspecified && StorageClassSpec == SCS_auto) {<br>
TypeSpecType = TST_auto;<br> StorageClassSpec = SCS_unspecified;<br>diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp<br>index 8bf5143..2038f12 100644<br>--- a/lib/Sema/SemaType.cpp<br>+++ b/lib/Sema/SemaType.cpp<br>
@@ -793,9 +793,7 @@ static QualType ConvertDeclSpecToType(TypeProcessingState &state) {<br> // "At least one type specifier shall be given in the declaration<br> // specifiers in each declaration, and in the specifier-qualifier list in<br>
// each struct declaration and type name."<br>- // FIXME: Does Microsoft really have the implicit int extension in C++?<br>- if (S.getLangOpts().CPlusPlus &&<br>- !S.getLangOpts().MicrosoftExt) {<br>
+ if (S.getLangOpts().CPlusPlus) {<br> S.Diag(DeclLoc, diag::err_missing_type_specifier)<br> << DS.getSourceRange();<br> <br>diff --git a/test/Rewriter/<a href="http://rewrite-byref-in-nested-blocks.mm" target="_blank">rewrite-byref-in-nested-blocks.mm</a> b/test/Rewriter/<a href="http://rewrite-byref-in-nested-blocks.mm" target="_blank">rewrite-byref-in-nested-blocks.mm</a><br>
index 022bb5f..f416b66 100644<br>--- a/test/Rewriter/<a href="http://rewrite-byref-in-nested-blocks.mm" target="_blank">rewrite-byref-in-nested-blocks.mm</a><br>+++ b/test/Rewriter/<a href="http://rewrite-byref-in-nested-blocks.mm" target="_blank">rewrite-byref-in-nested-blocks.mm</a><br>
@@ -19,7 +19,7 @@ void f(void (^block)(void));<br> - (void)foo {<br> __block int kerfluffle;<br> // radar 7692183<br>- __block x; <br>+ __block int x;<br> f(^{<br> f(^{<br>
y = 42;<div class="im"><br>diff --git a/test/Sema/MicrosoftCompatibility.cpp b/test/Sema/MicrosoftCompatibility.cpp<br>new file mode 100644<br>index 0000000..15c2558<br>--- /dev/null<br>+++ b/test/Sema/MicrosoftCompatibility.cpp<br>
@@ -0,0 +1,4 @@<br>+// RUN: %clang_cc1 %s -fsyntax-only -Wno-unused-value -Wmicrosoft -verify -fms-compatibility<br>+<br>+// PR15845<br>+int foo(xxx); // expected-error{{unknown type name}}<br><br clear="all"></div></div>
<span class=""><font color="#888888"><div class="gmail_extra">
<br>-- <br>Thanks,<br>--Serge<br>
</div></font></span></div>
</blockquote></div><br></div></div>