On 29 Apr 2013 20:01, "Aaron Ballman" <[email protected]> wrote: > > Correct; since we're going to be modifying the way this extension > works, it would be great if we could make it more compatible for > -fms-compatibility. In this case, MSVC handles void func(xxx) {} when > /TC is passed in, so it would be good for clang to do so as well > (along with the other cases that Richard pointed out).
That is unrelated to implicit int in c++, which the current patch addresses, and is in fact unrelated to implicit int at all - Serge confirmed that this is treated as an (ignored) identifier- list, not as a parameter with implicit int. Unless you have evidence that we need this extension, I don't see any reason to implement it. > ~Aaron > > On Mon, Apr 29, 2013 at 1:56 PM, Serge Pavlov <[email protected]> wrote: > > Clang now allows implicit int in C mode (-x c) in some cases: > > > > abc = 1; > > static foo() { return 1; } > > > > But in function declaration it is now forbidden unlike to GCC and MSVC: > > > > void func(xxx); > > > > --Serge > > > > > > > > > > 2013/4/30 Aaron Ballman <[email protected]> > >> > >> To be clear, MSVC does accept that code so long as you pass in the /TC > >> flag. So, for instance, the test cases with a .c or .m extension > >> should allow the implicit int (by default .c files are compiled with > >> /TC in MSVC), but the .cpp/.mm files should disallow it (unless we > >> force compilation as C with -x c, which would be the clang version of > >> /TC). > >> > >> ~Aaron > >> > >> On Mon, Apr 29, 2013 at 1:31 PM, 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. > >> > > >> >>> > >> >>> 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.mm > >> > b/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 > >> > > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
