On Mon, Apr 29, 2013 at 10:31 AM, Serge Pavlov <[email protected]> wrote:
> Hi Richard, > > > 2013/4/29 Richard Smith <[email protected]> > [...] > >> The problem is that clang in C++ mode accepts the code: >> >>> int foo(xxx); >>> Clang intentionally accepts this code due to a check in >>> Parser::ParseImplicitInt, which appeared in r156854. >>> 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. >>> >> >> 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... >> > > MSVC doesn't allow implicit int in any context if in C++ mode, details are > in bugzilla. > > >> > const n = 0; // ok? >> static f() { // ok? >> extern m; // ok? >> return m; >> } >> > > None of these cases are accepted by MSVC. > > >> 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. >> >> >>> 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. >>> >> >> Have you checked whether they treat it as an implicit int, or whether >> they treat it as an (ignored, presumably) identifier list? >> > > They are ignored. For instance, both MSVC and GCC successfully compile the > following code: > > void abc(xxx); > void abc(int x, char*y) {} > > >> Also, do you actually have code which relies upon this extension? If >> not, let's not add it gratuitously. >> > > I know nothing about such, the intent was to make behavior more > compatible. Probably it doesn't worth implementation. > > 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. >> > > 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. > Thanks, committed as r180822. > > >> Files: >>> include/clang/Basic/DiagnosticSemaKinds.td >>> lib/Parse/ParseDecl.cpp >>> lib/Sema/SemaType.cpp >>> test/Sema/MicrosoftCompatibility.cpp >>> test/Sema/alloc_size.c >>> test/Sema/function.c >>> test/Sema/invalid-decl.c >>> >>> >>> ----------------------------------------------------------------------------------------------------- >>> diff --git a/include/clang/Basic/DiagnosticSemaKinds.td >>> b/include/clang/Basic/DiagnosticSemaKinds.td >>> index 1461716..166dbab 100644 >>> --- a/include/clang/Basic/DiagnosticSemaKinds.td >>> +++ b/include/clang/Basic/DiagnosticSemaKinds.td >>> @@ -2314,8 +2314,9 @@ def err_void_only_param : Error< >>> "'void' must be the first and only parameter if specified">; >>> def err_void_param_qualified : Error< >>> "'void' as parameter must not have type qualifiers">; >>> -def err_ident_list_in_fn_declaration : Error< >>> - "a parameter list without types is only allowed in a function >>> definition">; >>> +def warn_ident_list_in_fn_declaration : Warning< >>> + "a parameter list without types is only allowed in a function >>> definition">, >>> + InGroup<C99Compat>; >>> >> >> This should be an ExtWarn, not a Warning, since this is a required >> diagnostic per the various C language standards. Also, C99Compat seems >> wrong. >> > > Thank you for the explanation. > > >> >>> def ext_param_not_declared : Extension< >>> "parameter %0 was not declared, defaulting to type 'int'">; >>> def err_param_typedef_of_void : Error< >>> diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp >>> index d786ce2..2f0c1a3 100644 >>> --- a/lib/Parse/ParseDecl.cpp >>> +++ b/lib/Parse/ParseDecl.cpp >>> @@ -2038,10 +2038,9 @@ bool Parser::ParseImplicitInt(DeclSpec &DS, >>> CXXScopeSpec *SS, >>> // error, do lookahead to try to do better recovery. This never >>> applies >>> // within a type specifier. Outside of C++, we allow this even if the >>> // language doesn't "officially" support implicit int -- we support >>> - // implicit int as an extension in C99 and C11. Allegedly, MS also >>> - // supports implicit int in C++ mode. >>> + // implicit int as an extension in C99 and C11. >>> if (DSC != DSC_type_specifier && DSC != DSC_trailing && >>> - (!getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt) && >>> + !getLangOpts().CPlusPlus && >>> >> >> 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. >> > > Indeed, SemaType.cpp also contains similar check. > > >> isValidAfterIdentifierInDeclarator(NextToken())) { >>> // If this token is valid for implicit int, e.g. "static x = 4", >>> then >>> // we just avoid eating the identifier, so it will be parsed as the >>> diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp >>> index 8bf5143..243b772 100644 >>> --- a/lib/Sema/SemaType.cpp >>> +++ b/lib/Sema/SemaType.cpp >>> @@ -2742,7 +2742,7 @@ static TypeSourceInfo >>> *GetFullTypeForDeclarator(TypeProcessingState &state, >>> if (FTI.NumArgs && FTI.ArgInfo[0].Param == 0) { >>> // C99 6.7.5.3p3: Reject int(x,y,z) when it's not a function >>> // definition. >>> - S.Diag(FTI.ArgInfo[0].IdentLoc, >>> diag::err_ident_list_in_fn_declaration); >>> + S.Diag(FTI.ArgInfo[0].IdentLoc, >>> diag::warn_ident_list_in_fn_declaration); >>> D.setInvalidType(true); >>> >> >> If you're not issuing an error, you must build a correct AST -- you can't >> set things invalid. >> >> > My fault... > [...] > > > Updated patch: > > Files: > lib/Parse/ParseDecl.cpp > lib/Sema/DeclSpec.cpp > lib/Sema/SemaType.cpp > test/Rewriter/rewrite-byref-in-nested-blocks.mm > test/Sema/MicrosoftCompatibility.cpp > > > diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp > index d786ce2..2f0c1a3 100644 > --- a/lib/Parse/ParseDecl.cpp > +++ b/lib/Parse/ParseDecl.cpp > @@ -2038,10 +2038,9 @@ bool Parser::ParseImplicitInt(DeclSpec &DS, > CXXScopeSpec *SS, > // error, do lookahead to try to do better recovery. This never applies > // within a type specifier. Outside of C++, we allow this even if the > // language doesn't "officially" support implicit int -- we support > - // implicit int as an extension in C99 and C11. Allegedly, MS also > - // supports implicit int in C++ mode. > + // implicit int as an extension in C99 and C11. > if (DSC != DSC_type_specifier && DSC != DSC_trailing && > - (!getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt) && > + !getLangOpts().CPlusPlus && > isValidAfterIdentifierInDeclarator(NextToken())) { > // If this token is valid for implicit int, e.g. "static x = 4", then > // we just avoid eating the identifier, so it will be parsed as the > diff --git a/lib/Sema/DeclSpec.cpp b/lib/Sema/DeclSpec.cpp > index 124f50c..3b3ab2c 100644 > --- a/lib/Sema/DeclSpec.cpp > +++ b/lib/Sema/DeclSpec.cpp > @@ -1003,8 +1003,7 @@ void DeclSpec::Finish(DiagnosticsEngine &D, > Preprocessor &PP) { > // the type specifier is not optional, but we got 'auto' as a storage > // class specifier, then assume this is an attempt to use C++0x's 'auto' > // type specifier. > - // FIXME: Does Microsoft really support implicit int in C++? > - if (PP.getLangOpts().CPlusPlus && !PP.getLangOpts().MicrosoftExt && > + if (PP.getLangOpts().CPlusPlus && > TypeSpecType == TST_unspecified && StorageClassSpec == SCS_auto) { > TypeSpecType = TST_auto; > StorageClassSpec = SCS_unspecified; > diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp > index 8bf5143..2038f12 100644 > --- a/lib/Sema/SemaType.cpp > +++ b/lib/Sema/SemaType.cpp > @@ -793,9 +793,7 @@ static QualType > ConvertDeclSpecToType(TypeProcessingState &state) { > // "At least one type specifier shall be given in the declaration > // specifiers in each declaration, and in the specifier-qualifier > list in > // each struct declaration and type name." > - // FIXME: Does Microsoft really have the implicit int extension in > C++? > - if (S.getLangOpts().CPlusPlus && > - !S.getLangOpts().MicrosoftExt) { > + if (S.getLangOpts().CPlusPlus) { > S.Diag(DeclLoc, diag::err_missing_type_specifier) > << DS.getSourceRange(); > > diff --git a/test/Rewriter/rewrite-byref-in-nested-blocks.mmb/test/Rewriter/ > rewrite-byref-in-nested-blocks.mm > index 022bb5f..f416b66 100644 > --- a/test/Rewriter/rewrite-byref-in-nested-blocks.mm > +++ b/test/Rewriter/rewrite-byref-in-nested-blocks.mm > @@ -19,7 +19,7 @@ void f(void (^block)(void)); > - (void)foo { > __block int kerfluffle; > // radar 7692183 > - __block x; > + __block int x; > f(^{ > f(^{ > y = 42; > > diff --git a/test/Sema/MicrosoftCompatibility.cpp > b/test/Sema/MicrosoftCompatibility.cpp > new file mode 100644 > index 0000000..15c2558 > --- /dev/null > +++ b/test/Sema/MicrosoftCompatibility.cpp > @@ -0,0 +1,4 @@ > +// RUN: %clang_cc1 %s -fsyntax-only -Wno-unused-value -Wmicrosoft -verify > -fms-compatibility > + > +// PR15845 > +int foo(xxx); // expected-error{{unknown type name}} > > > -- > Thanks, > --Serge >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
