I agree with Nico in retrospect. Setting -fno-operator-names is too big a hammer. Instead we should add some MSVCCompat hacks to pretend we saw an identifier token instead of a keyword token when these keywords are used as declarators.
On Thu, May 25, 2017 at 10:14 AM, Nico Weber <tha...@google.com> wrote: > Among the goals of the clang-cl project are a) being able to parse MS > system headers b) helper users to write standards-compliant C++ (in > particular, if your code builds with clang-cl without warnings, it'd be > good if it also built with regular clang then). > > The regular change plus header change plus warning would be one way to > achieve this, yes. > > On Thu, May 25, 2017 at 1:01 PM, Keane, Erich <erich.ke...@intel.com> > wrote: > >> No problem, I definitely think it was the right choice. >> >> >> >> A change that contains all of what Melanie’s original patch did, plus the >> Header changes (plus the clang-tidy fixes that came out of this) would be >> acceptable? >> >> >> >> Also, I believe she’s working on the warning as well. We were discussing >> it, and I was thinking that if we track down where the >> operator-identification is done (hints as to where that is is appreciated), >> that we could simply warn if the user is using one of the Operators with >> operators off. The warning itself could then be suppressed if necessary. >> >> >> >> Thoughts? >> >> >> >> >> >> *From:* Nico Weber [mailto:tha...@google.com] >> *Sent:* Thursday, May 25, 2017 9:56 AM >> *To:* Keane, Erich <erich.ke...@intel.com> >> *Cc:* Blower, Melanie <melanie.blo...@intel.com>; rnk <r...@chromium.org>; >> cfe-commits <cfe-commits@lists.llvm.org>; Hans Wennborg < >> h...@chromium.org> >> >> *Subject:* Re: r303798 - For Microsoft compatibility, set >> fno_operator_names >> >> >> >> On Thu, May 25, 2017 at 12:18 PM, Keane, Erich <erich.ke...@intel.com> >> wrote: >> >> How does chromium compiler in CL? It seems that it would have a similar >> problem here… >> >> >> >> That's a good question! It looks like iso646.h is included, and in MSVC >> that contains something like `#define and &&`. But clang's >> lib/Headers/iso646.h assumes that the compiler provides this and doesn't >> define `and`. So for the reland, that header would have to check if the >> operator name is disabled, and if so, define it. (Or, better, maybe we can >> come up with something more targeted for the system header, similar to e.g. >> http://llvm.org/viewvc/llvm-project?view=revision&revision=212238) >> >> >> >> Thanks for the revert! >> >> >> >> >> >> *From:* tha...@google.com [mailto:tha...@google.com] *On Behalf Of *Nico >> Weber >> *Sent:* Thursday, May 25, 2017 9:16 AM >> *To:* Blower, Melanie <melanie.blo...@intel.com> >> *Cc:* rnk <r...@chromium.org>; Keane, Erich <erich.ke...@intel.com>; >> cfe-commits <cfe-commits@lists.llvm.org>; Hans Wennborg < >> h...@chromium.org> >> >> >> *Subject:* RE: r303798 - For Microsoft compatibility, set >> fno_operator_names >> >> >> >> In addition to this making clang-cl silently accept invalid code, it also >> breaks existing valid code, building chromium now fails. Let's revert and >> come up with something better asynchronously. >> >> >> >> FAILED: obj/third_party/WebKit/Source/core/dom/dom/CustomElementRegistry.obj >> >> >> E:\b\c\goma_client/gomacc.exe ../../third_party/llvm-build/R >> elease+Asserts/bin/clang-cl.exe /nologo /showIncludes /FC >> @obj/third_party/WebKit/Source/core/dom/dom/CustomElementRegistry.obj.rsp >> /c ../../third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.cpp >> /Foobj/third_party/WebKit/Source/core/dom/dom/CustomElementRegistry.obj >> /Fd"obj/third_party/WebKit/Source/core/dom/dom_cc.pdb" >> >> E:\b\c\b\win_clang\src\third_party\WebKit\Source\core\dom\cu >> stom\CustomElementRegistry.cpp(229,7): error: unknown type name >> 'definition' >> >> if (definition and definition->Descriptor().LocalName() == >> desc.LocalName()) { >> >> ^ >> >> E:\b\c\b\win_clang\src\third_party\WebKit\Source\core\dom\cu >> stom\CustomElementRegistry.cpp(229,18): error: variable declaration in >> condition must have an initializer >> >> if (definition and definition->Descriptor().LocalName() == >> desc.LocalName()) { >> >> ^ >> >> 2 errors generated. >> >> >> >> >> >> On May 24, 2017 4:01 PM, "Blower, Melanie" <melanie.blo...@intel.com> >> wrote: >> >> Thanks for the feedback, working on it… >> >> >> >> *From:* Keane, Erich >> *Sent:* Wednesday, May 24, 2017 3:47 PM >> *To:* Nico Weber <tha...@chromium.org>; Blower, Melanie < >> melanie.blo...@intel.com> >> >> >> *Cc:* cfe-commits <cfe-commits@lists.llvm.org>; rnk <r...@chromium.org> >> >> *Subject:* RE: r303798 - For Microsoft compatibility, set >> fno_operator_names >> >> >> >> Adding Melanie, the author of the patch. >> >> >> >> *From:* tha...@google.com [mailto:tha...@google.com <tha...@google.com>] *On >> Behalf Of *Nico Weber >> *Sent:* Wednesday, May 24, 2017 12:43 PM >> *To:* Keane, Erich <erich.ke...@intel.com> >> *Cc:* cfe-commits <cfe-commits@lists.llvm.org>; rnk <r...@chromium.org> >> *Subject:* Re: r303798 - For Microsoft compatibility, set >> fno_operator_names >> >> >> >> Reviewed here: https://reviews.llvm.org/D33505 >> >> >> Still, please make this warn. >> >> >> >> On Wed, May 24, 2017 at 3:42 PM, Nico Weber <tha...@google.com> wrote: >> >> Was this reviewed somewhere? >> >> >> >> Please make it so that this emits a warning. We want clang-cl to warn on >> invalid code (and in system headers warnings are suppressed). >> >> >> >> On Wed, May 24, 2017 at 3:31 PM, Erich Keane via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >> Author: erichkeane >> Date: Wed May 24 14:31:19 2017 >> New Revision: 303798 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=303798&view=rev >> Log: >> For Microsoft compatibility, set fno_operator_names >> >> There's a Microsoft header in the Windows SDK which won't >> compile with clang because it uses an operator name (and) >> as a field name. This patch allows that file to compile by >> setting the option which disables operator names. >> The header which doesn't compile <Query.h> C:/Program Files (x86)/ >> Windows Kits/10/include/10.0.14393.0/um\Query.h:259:40: >> error: expected member name or ';' after declaration specifiers >> >> /* [case()] */ NODERESTRICTION or; >> ~~~~~~~~~~~~~~~ ^ >> >> 1 error generated. >> >> Contributed for Melanie Blower >> >> Differential Revision:https://reviews.llvm.org/D33505 >> >> Modified: >> cfe/trunk/lib/Frontend/CompilerInvocation.cpp >> cfe/trunk/test/Parser/MicrosoftExtensions.cpp >> cfe/trunk/test/Preprocessor/cxx_oper_keyword.cpp >> >> Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp >> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ >> CompilerInvocation.cpp?rev=303798&r1=303797&r2=303798&view=diff >> ============================================================ >> ================== >> --- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original) >> +++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Wed May 24 14:31:19 >> 2017 >> @@ -1882,7 +1882,7 @@ static void ParseLangArgs(LangOptions &O >> Opts.GNUKeywords = Args.hasFlag(OPT_fgnu_keywords, >> OPT_fno_gnu_keywords, >> Opts.GNUKeywords); >> >> - if (Args.hasArg(OPT_fno_operator_names)) >> + if (Args.hasArg(OPT_fno_operator_names) || >> Args.hasArg(OPT_fms_compatibility)) >> Opts.CXXOperatorNames = 0; >> >> if (Args.hasArg(OPT_fcuda_is_device)) >> >> Modified: cfe/trunk/test/Parser/MicrosoftExtensions.cpp >> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/Mi >> crosoftExtensions.cpp?rev=303798&r1=303797&r2=303798&view=diff >> ============================================================ >> ================== >> --- cfe/trunk/test/Parser/MicrosoftExtensions.cpp (original) >> +++ cfe/trunk/test/Parser/MicrosoftExtensions.cpp Wed May 24 14:31:19 >> 2017 >> @@ -261,9 +261,8 @@ int __identifier(else} = __identifier(fo >> #define identifier_weird(x) __identifier(x >> int k = identifier_weird(if)); // expected-error {{use of undeclared >> identifier 'if'}} >> >> -// This is a bit weird, but the alternative tokens aren't keywords, and >> this >> -// behavior matches MSVC. FIXME: Consider supporting this anyway. >> -extern int __identifier(and) r; // expected-error {{cannot convert '&&' >> token to an identifier}} >> +// 'and' is not an operator name with Microsoft compatibility. >> +extern int __identifier(and) r; // expected-error {{expected ';' after >> top level declarator}} >> >> void f() { >> __identifier(() // expected-error {{cannot convert '(' token to an >> identifier}} >> @@ -355,7 +354,6 @@ void TestProperty() { >> ++sp.V11; >> } >> >> -//expected-warning@+1 {{C++ operator 'and' (aka '&&') used as a macro >> name}} >> #define and foo >> >> struct __declspec(uuid("00000000-0000-0000-C000-000000000046")) >> __declspec(novtable) IUnknown {}; >> >> Modified: cfe/trunk/test/Preprocessor/cxx_oper_keyword.cpp >> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Preproces >> sor/cxx_oper_keyword.cpp?rev=303798&r1=303797&r2=303798&view=diff >> ============================================================ >> ================== >> --- cfe/trunk/test/Preprocessor/cxx_oper_keyword.cpp (original) >> +++ cfe/trunk/test/Preprocessor/cxx_oper_keyword.cpp Wed May 24 14:31:19 >> 2017 >> @@ -1,5 +1,6 @@ >> // RUN: %clang_cc1 %s -E -verify -DOPERATOR_NAMES >> // RUN: %clang_cc1 %s -E -verify -fno-operator-names >> +// RUN: %clang_cc1 %s -verify -DTESTWIN -fms-compatibility >> >> #ifndef OPERATOR_NAMES >> //expected-error@+3 {{token is not a valid binary operator in a >> preprocessor subexpression}} >> @@ -29,3 +30,14 @@ >> #ifdef and >> #warning and is defined >> #endif >> + >> +#ifdef TESTWIN >> +// For cl compatibility, fno-operator-names is enabled by default. >> +int and; >> +int bitand; >> +int bitor; >> +int compl; >> +int not; >> +int or; >> +int xor; >> +#endif >> >> >> _______________________________________________ >> 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