Re: r292508 - Module: Improve diagnostic message when cxx modules are disabled and @import is used in Objective CXX.
On 20 Jan 2017 9:49 am, "Manman Ren" wrote: On Thu, Jan 19, 2017 at 1:27 PM, Richard Smith wrote: > On 19 Jan 2017 11:16 am, "Manman Ren via cfe-commits" < > cfe-commits@lists.llvm.org> wrote: > > Author: mren > Date: Thu Jan 19 13:05:55 2017 > New Revision: 292508 > > URL: http://llvm.org/viewvc/llvm-project?rev=292508&view=rev > Log: > Module: Improve diagnostic message when cxx modules are disabled and > @import is used in Objective CXX. > > rdar://problem/19399671 > > Added: > cfe/trunk/test/Modules/check-syntax.mm > Modified: > cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td > cfe/trunk/lib/Parse/ParseObjc.cpp > > Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ > Basic/DiagnosticParseKinds.td?rev=292508&r1=292507&r2=292508&view=diff > > == > --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Thu Jan 19 > 13:05:55 2017 > @@ -243,7 +243,10 @@ def err_expected_property_name : Error<" > > def err_unexpected_at : Error<"unexpected '@' in program">; > def err_atimport : Error< > -"use of '@import' when modules are disabled">; > + "use of '@import' when modules are disabled">; > +def err_atimport_cxx : Error< > + "use of '@import' when C++ modules are disabled, consider using > fmodules " > + "and fcxx-modules">; > > > Please add a hyphen before fmodules. There's also no reason to suggest > -fcxx-modules; modules support for C++ is controlled by -fmodules nowadays. > > I also don't think it makes sense to have different diagnostics for C and > C++ mode; we should suggest the -fmodules flag in either case. > Hi Richard, This is an internal patch that I am upstreaming to open source. Internally at Apple, modules support for C++ is off by default, users need to use -fcxx-modules to turn it on. So when they use -fmodules without -fcxx-modules, they will get a confusing error saying modules are disabled. Open source, the testing case in this commit uses driver option -fno-cxx-modules, in this case, it makes sense to call out c++ modules so users know to remove -fno-cxx-modules. I think it's time to remove -fno-cxx-modules entirely from upstream. If we don't, I don't think it makes any sense to mention that flag unless the user actually specified it. Let me know your thoughts, Manman > > def err_invalid_reference_qualifier_application : Error< >"'%0' qualifier may not be applied to a reference">; > > Modified: cfe/trunk/lib/Parse/ParseObjc.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/Pars > eObjc.cpp?rev=292508&r1=292507&r2=292508&view=diff > > == > --- cfe/trunk/lib/Parse/ParseObjc.cpp (original) > +++ cfe/trunk/lib/Parse/ParseObjc.cpp Thu Jan 19 13:05:55 2017 > @@ -83,7 +83,10 @@ Parser::DeclGroupPtrTy Parser::ParseObjC >case tok::objc_import: > if (getLangOpts().Modules || getLangOpts().DebuggerSupport) >return ParseModuleImport(AtLoc); > -Diag(AtLoc, diag::err_atimport); > +if (getLangOpts().CPlusPlus) > + Diag(AtLoc, diag::err_atimport_cxx); > +else > + Diag(AtLoc, diag::err_atimport); > SkipUntil(tok::semi); > return Actions.ConvertDeclToDeclGroup(nullptr); >default: > > Added: cfe/trunk/test/Modules/check-syntax.mm > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/c > heck-syntax.mm?rev=292508&view=auto > > == > --- cfe/trunk/test/Modules/check-syntax.mm (added) > +++ cfe/trunk/test/Modules/check-syntax.mm Thu Jan 19 13:05:55 2017 > @@ -0,0 +1,5 @@ > +// RUN: not %clang -fmodules -fno-cxx-modules -fsyntax-only %s 2>&1 | > FileCheck %s > +// rdar://19399671 > + > +// CHECK: use of '@import' when C++ modules are disabled > +@import Foundation; > > > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r292508 - Module: Improve diagnostic message when cxx modules are disabled and @import is used in Objective CXX.
On Thu, Jan 19, 2017 at 1:27 PM, Richard Smith wrote: > On 19 Jan 2017 11:16 am, "Manman Ren via cfe-commits" < > cfe-commits@lists.llvm.org> wrote: > > Author: mren > Date: Thu Jan 19 13:05:55 2017 > New Revision: 292508 > > URL: http://llvm.org/viewvc/llvm-project?rev=292508&view=rev > Log: > Module: Improve diagnostic message when cxx modules are disabled and > @import is used in Objective CXX. > > rdar://problem/19399671 > > Added: > cfe/trunk/test/Modules/check-syntax.mm > Modified: > cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td > cfe/trunk/lib/Parse/ParseObjc.cpp > > Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ > Basic/DiagnosticParseKinds.td?rev=292508&r1=292507&r2=292508&view=diff > > == > --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Thu Jan 19 > 13:05:55 2017 > @@ -243,7 +243,10 @@ def err_expected_property_name : Error<" > > def err_unexpected_at : Error<"unexpected '@' in program">; > def err_atimport : Error< > -"use of '@import' when modules are disabled">; > + "use of '@import' when modules are disabled">; > +def err_atimport_cxx : Error< > + "use of '@import' when C++ modules are disabled, consider using > fmodules " > + "and fcxx-modules">; > > > Please add a hyphen before fmodules. There's also no reason to suggest > -fcxx-modules; modules support for C++ is controlled by -fmodules nowadays. > > I also don't think it makes sense to have different diagnostics for C and > C++ mode; we should suggest the -fmodules flag in either case. > Hi Richard, This is an internal patch that I am upstreaming to open source. Internally at Apple, modules support for C++ is off by default, users need to use -fcxx-modules to turn it on. So when they use -fmodules without -fcxx-modules, they will get a confusing error saying modules are disabled. Open source, the testing case in this commit uses driver option -fno-cxx-modules, in this case, it makes sense to call out c++ modules so users know to remove -fno-cxx-modules. Let me know your thoughts, Manman > > def err_invalid_reference_qualifier_application : Error< >"'%0' qualifier may not be applied to a reference">; > > Modified: cfe/trunk/lib/Parse/ParseObjc.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/Pars > eObjc.cpp?rev=292508&r1=292507&r2=292508&view=diff > > == > --- cfe/trunk/lib/Parse/ParseObjc.cpp (original) > +++ cfe/trunk/lib/Parse/ParseObjc.cpp Thu Jan 19 13:05:55 2017 > @@ -83,7 +83,10 @@ Parser::DeclGroupPtrTy Parser::ParseObjC >case tok::objc_import: > if (getLangOpts().Modules || getLangOpts().DebuggerSupport) >return ParseModuleImport(AtLoc); > -Diag(AtLoc, diag::err_atimport); > +if (getLangOpts().CPlusPlus) > + Diag(AtLoc, diag::err_atimport_cxx); > +else > + Diag(AtLoc, diag::err_atimport); > SkipUntil(tok::semi); > return Actions.ConvertDeclToDeclGroup(nullptr); >default: > > Added: cfe/trunk/test/Modules/check-syntax.mm > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/ > check-syntax.mm?rev=292508&view=auto > > == > --- cfe/trunk/test/Modules/check-syntax.mm (added) > +++ cfe/trunk/test/Modules/check-syntax.mm Thu Jan 19 13:05:55 2017 > @@ -0,0 +1,5 @@ > +// RUN: not %clang -fmodules -fno-cxx-modules -fsyntax-only %s 2>&1 | > FileCheck %s > +// rdar://19399671 > + > +// CHECK: use of '@import' when C++ modules are disabled > +@import Foundation; > > > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r292508 - Module: Improve diagnostic message when cxx modules are disabled and @import is used in Objective CXX.
On 19 Jan 2017 11:16 am, "Manman Ren via cfe-commits" < cfe-commits@lists.llvm.org> wrote: Author: mren Date: Thu Jan 19 13:05:55 2017 New Revision: 292508 URL: http://llvm.org/viewvc/llvm-project?rev=292508&view=rev Log: Module: Improve diagnostic message when cxx modules are disabled and @import is used in Objective CXX. rdar://problem/19399671 Added: cfe/trunk/test/Modules/check-syntax.mm Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td cfe/trunk/lib/Parse/ParseObjc.cpp Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/ DiagnosticParseKinds.td?rev=292508&r1=292507&r2=292508&view=diff == --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original) +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Thu Jan 19 13:05:55 2017 @@ -243,7 +243,10 @@ def err_expected_property_name : Error<" def err_unexpected_at : Error<"unexpected '@' in program">; def err_atimport : Error< -"use of '@import' when modules are disabled">; + "use of '@import' when modules are disabled">; +def err_atimport_cxx : Error< + "use of '@import' when C++ modules are disabled, consider using fmodules " + "and fcxx-modules">; def err_invalid_reference_qualifier_application : Error< "'%0' qualifier may not be applied to a reference">; Modified: cfe/trunk/lib/Parse/ParseObjc.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ ParseObjc.cpp?rev=292508&r1=292507&r2=292508&view=diff == --- cfe/trunk/lib/Parse/ParseObjc.cpp (original) +++ cfe/trunk/lib/Parse/ParseObjc.cpp Thu Jan 19 13:05:55 2017 @@ -83,7 +83,10 @@ Parser::DeclGroupPtrTy Parser::ParseObjC case tok::objc_import: if (getLangOpts().Modules || getLangOpts().DebuggerSupport) return ParseModuleImport(AtLoc); -Diag(AtLoc, diag::err_atimport); +if (getLangOpts().CPlusPlus) + Diag(AtLoc, diag::err_atimport_cxx); +else + Diag(AtLoc, diag::err_atimport); SkipUntil(tok::semi); return Actions.ConvertDeclToDeclGroup(nullptr); default: Added: cfe/trunk/test/Modules/check-syntax.mm URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ Modules/check-syntax.mm?rev=292508&view=auto == --- cfe/trunk/test/Modules/check-syntax.mm (added) +++ cfe/trunk/test/Modules/check-syntax.mm Thu Jan 19 13:05:55 2017 @@ -0,0 +1,5 @@ +// RUN: not %clang -fmodules -fno-cxx-modules -fsyntax-only %s 2>&1 | FileCheck %s +// rdar://19399671 + +// CHECK: use of '@import' when C++ modules are disabled Please test for diagnostics with -verify rather than FileCheck when possible. +@import Foundation; ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r292508 - Module: Improve diagnostic message when cxx modules are disabled and @import is used in Objective CXX.
On 19 Jan 2017 11:16 am, "Manman Ren via cfe-commits" < cfe-commits@lists.llvm.org> wrote: Author: mren Date: Thu Jan 19 13:05:55 2017 New Revision: 292508 URL: http://llvm.org/viewvc/llvm-project?rev=292508&view=rev Log: Module: Improve diagnostic message when cxx modules are disabled and @import is used in Objective CXX. rdar://problem/19399671 Added: cfe/trunk/test/Modules/check-syntax.mm Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td cfe/trunk/lib/Parse/ParseObjc.cpp Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/ DiagnosticParseKinds.td?rev=292508&r1=292507&r2=292508&view=diff == --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original) +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Thu Jan 19 13:05:55 2017 @@ -243,7 +243,10 @@ def err_expected_property_name : Error<" def err_unexpected_at : Error<"unexpected '@' in program">; def err_atimport : Error< -"use of '@import' when modules are disabled">; + "use of '@import' when modules are disabled">; +def err_atimport_cxx : Error< + "use of '@import' when C++ modules are disabled, consider using fmodules " + "and fcxx-modules">; Please add a hyphen before fmodules. There's also no reason to suggest -fcxx-modules; modules support for C++ is controlled by -fmodules nowadays. I also don't think it makes sense to have different diagnostics for C and C++ mode; we should suggest the -fmodules flag in either case. def err_invalid_reference_qualifier_application : Error< "'%0' qualifier may not be applied to a reference">; Modified: cfe/trunk/lib/Parse/ParseObjc.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ ParseObjc.cpp?rev=292508&r1=292507&r2=292508&view=diff == --- cfe/trunk/lib/Parse/ParseObjc.cpp (original) +++ cfe/trunk/lib/Parse/ParseObjc.cpp Thu Jan 19 13:05:55 2017 @@ -83,7 +83,10 @@ Parser::DeclGroupPtrTy Parser::ParseObjC case tok::objc_import: if (getLangOpts().Modules || getLangOpts().DebuggerSupport) return ParseModuleImport(AtLoc); -Diag(AtLoc, diag::err_atimport); +if (getLangOpts().CPlusPlus) + Diag(AtLoc, diag::err_atimport_cxx); +else + Diag(AtLoc, diag::err_atimport); SkipUntil(tok::semi); return Actions.ConvertDeclToDeclGroup(nullptr); default: Added: cfe/trunk/test/Modules/check-syntax.mm URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ Modules/check-syntax.mm?rev=292508&view=auto == --- cfe/trunk/test/Modules/check-syntax.mm (added) +++ cfe/trunk/test/Modules/check-syntax.mm Thu Jan 19 13:05:55 2017 @@ -0,0 +1,5 @@ +// RUN: not %clang -fmodules -fno-cxx-modules -fsyntax-only %s 2>&1 | FileCheck %s +// rdar://19399671 + +// CHECK: use of '@import' when C++ modules are disabled +@import Foundation; ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
