[PATCH] D103773: [clang-cl] Add /permissive and /permissive-
zero9178 marked an inline comment as done. zero9178 added inline comments. Comment at: clang/test/Driver/cl-permissive.c:15 +// PERMISSIVE-OVERWRITE-NOT: "-fdelayed-template-parsing" +// RUN: %clang_cl /permissive- /Zc:twoPhase- -### -- %s 2>&1 | FileCheck -check-prefix=PERMISSIVE-MINUS-OVERWRITE %s +// PERMISSIVE-MINUS-OVERWRITE-NOT: "-fno-operator-names" zero9178 wrote: > aganea wrote: > > Hello @zero9178 ! > > After this patch, the following fails: > > `// RUN: %clang_cl /permissive- -- %s` > > generates: > > `error: error reading '/Zc:strictStrings'` > > Because `/Zc:strictStrings` isn't correctly expanded to > > `-Werror=c++11-compat-deprecated-writable-strings` as it should, but it is > > passed as-is to cc1. You can put a breakpoint here: > > https://github.com/llvm/llvm-project/blob/fc018ebb608ee0c1239b405460e49f1835ab6175/clang/lib/Driver/ToolChains/Clang.cpp#L5374 > > and observe before/after this line. > > Would you possibly have a chance to take a look please? > Absolutely! I will take a look as soon as possible which will be on the > weekend. Meanwhile feel free to either revert the commit or remove the > expansion to /Zc:strictStrings I have removed the expansion to strictStrings in https://reviews.llvm.org/rGc9889c44ec5a4054833457c813e155f284703ef4. Apparently when AdAllArgs is called it does not correctly render it as I thought it would. The fix isn't very obvious to me, and my time is sadly limited. In hindsight it is also probably not a good idea to add strictStrings until strictStrings exists/is neeeded. Hope this helps. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103773/new/ https://reviews.llvm.org/D103773 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D103773: [clang-cl] Add /permissive and /permissive-
zero9178 added inline comments. Comment at: clang/test/Driver/cl-permissive.c:15 +// PERMISSIVE-OVERWRITE-NOT: "-fdelayed-template-parsing" +// RUN: %clang_cl /permissive- /Zc:twoPhase- -### -- %s 2>&1 | FileCheck -check-prefix=PERMISSIVE-MINUS-OVERWRITE %s +// PERMISSIVE-MINUS-OVERWRITE-NOT: "-fno-operator-names" aganea wrote: > Hello @zero9178 ! > After this patch, the following fails: > `// RUN: %clang_cl /permissive- -- %s` > generates: > `error: error reading '/Zc:strictStrings'` > Because `/Zc:strictStrings` isn't correctly expanded to > `-Werror=c++11-compat-deprecated-writable-strings` as it should, but it is > passed as-is to cc1. You can put a breakpoint here: > https://github.com/llvm/llvm-project/blob/fc018ebb608ee0c1239b405460e49f1835ab6175/clang/lib/Driver/ToolChains/Clang.cpp#L5374 > and observe before/after this line. > Would you possibly have a chance to take a look please? Absolutely! I will take a look as soon as possible which will be on the weekend. Meanwhile feel free to either revert the commit or remove the expansion to /Zc:strictStrings Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103773/new/ https://reviews.llvm.org/D103773 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D103773: [clang-cl] Add /permissive and /permissive-
aganea added inline comments. Comment at: clang/test/Driver/cl-permissive.c:15 +// PERMISSIVE-OVERWRITE-NOT: "-fdelayed-template-parsing" +// RUN: %clang_cl /permissive- /Zc:twoPhase- -### -- %s 2>&1 | FileCheck -check-prefix=PERMISSIVE-MINUS-OVERWRITE %s +// PERMISSIVE-MINUS-OVERWRITE-NOT: "-fno-operator-names" Hello @zero9178 ! After this patch, the following fails: `// RUN: %clang_cl /permissive- -- %s` generates: `error: error reading '/Zc:strictStrings'` Because `/Zc:strictStrings` isn't correctly expanded to `-Werror=c++11-compat-deprecated-writable-strings` as it should, but it is passed as-is to cc1. You can put a breakpoint here: https://github.com/llvm/llvm-project/blob/fc018ebb608ee0c1239b405460e49f1835ab6175/clang/lib/Driver/ToolChains/Clang.cpp#L5374 and observe before/after this line. Would you possibly have a chance to take a look please? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103773/new/ https://reviews.llvm.org/D103773 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D103773: [clang-cl] Add /permissive and /permissive-
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc70b0e808da8: [clang-cl] Add /permissive and /permissive-
(authored by zero9178).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103773/new/
https://reviews.llvm.org/D103773
Files:
clang/include/clang/Driver/Options.td
clang/lib/Driver/ToolChains/MSVC.cpp
clang/test/Driver/cl-permissive.c
Index: clang/test/Driver/cl-permissive.c
===
--- /dev/null
+++ clang/test/Driver/cl-permissive.c
@@ -0,0 +1,17 @@
+// Note: %s must be preceded by --, otherwise it may be interpreted as a
+// command-line option, e.g. on Mac where %s is commonly under /Users.
+
+// RUN: %clang_cl /permissive -### -- %s 2>&1 | FileCheck
-check-prefix=PERMISSIVE %s
+// PERMISSIVE: "-fno-operator-names"
+// PERMISSIVE: "-fdelayed-template-parsing"
+// RUN: %clang_cl /permissive- -### -- %s 2>&1 | FileCheck
-check-prefix=PERMISSIVE-MINUS %s
+// PERMISSIVE-MINUS-NOT: "-fno-operator-names"
+// PERMISSIVE-MINUS-NOT: "-fdelayed-template-parsing"
+
+// The switches set by permissive may then still be manually enabled or
disabled
+// RUN: %clang_cl /permissive /Zc:twoPhase -### -- %s 2>&1 | FileCheck
-check-prefix=PERMISSIVE-OVERWRITE %s
+// PERMISSIVE-OVERWRITE: "-fno-operator-names"
+// PERMISSIVE-OVERWRITE-NOT: "-fdelayed-template-parsing"
+// RUN: %clang_cl /permissive- /Zc:twoPhase- -### -- %s 2>&1 | FileCheck
-check-prefix=PERMISSIVE-MINUS-OVERWRITE %s
+// PERMISSIVE-MINUS-OVERWRITE-NOT: "-fno-operator-names"
+// PERMISSIVE-MINUS-OVERWRITE: "-fdelayed-template-parsing"
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -1523,6 +1523,20 @@
DAL.AddJoinedArg(A, Opts.getOption(options::OPT_D), NewVal);
}
+static void TranslatePermissive(Arg *A, llvm::opt::DerivedArgList &DAL,
+const OptTable &Opts) {
+ DAL.AddFlagArg(A, Opts.getOption(options::OPT__SLASH_Zc_twoPhase_));
+ DAL.AddFlagArg(A, Opts.getOption(options::OPT_fno_operator_names));
+ // There is currently no /Zc:strictStrings- in clang-cl
+}
+
+static void TranslatePermissiveMinus(Arg *A, llvm::opt::DerivedArgList &DAL,
+ const OptTable &Opts) {
+ DAL.AddFlagArg(A, Opts.getOption(options::OPT__SLASH_Zc_twoPhase));
+ DAL.AddFlagArg(A, Opts.getOption(options::OPT_foperator_names));
+ DAL.AddFlagArg(A, Opts.getOption(options::OPT__SLASH_Zc_strictStrings));
+}
+
llvm::opt::DerivedArgList *
MSVCToolChain::TranslateArgs(const llvm::opt::DerivedArgList &Args,
StringRef BoundArch,
@@ -1565,6 +1579,12 @@
} else if (A->getOption().matches(options::OPT_D)) {
// Translate -Dfoo#bar into -Dfoo=bar.
TranslateDArg(A, *DAL, Opts);
+} else if (A->getOption().matches(options::OPT__SLASH_permissive)) {
+ // Expand /permissive
+ TranslatePermissive(A, *DAL, Opts);
+} else if (A->getOption().matches(options::OPT__SLASH_permissive_)) {
+ // Expand /permissive-
+ TranslatePermissiveMinus(A, *DAL, Opts);
} else if (OFK != Action::OFK_HIP) {
// HIP Toolchain translates input args by itself.
DAL->append(A);
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -6099,6 +6099,10 @@
HelpText<"Deprecated (set output file name); use /Fe or /Fe">,
MetaVarName<"">;
def _SLASH_P : CLFlag<"P">, HelpText<"Preprocess to file">;
+def _SLASH_permissive : CLFlag<"permissive">,
+ HelpText<"Enable some non conforming code to compile">;
+def _SLASH_permissive_ : CLFlag<"permissive-">,
+ HelpText<"Disable non conforming code from compiling (default)">;
def _SLASH_Tc : CLCompileJoinedOrSeparate<"Tc">,
HelpText<"Treat as C source file">, MetaVarName<"">;
def _SLASH_TC : CLCompileFlag<"TC">, HelpText<"Treat all source files as C">;
@@ -6180,7 +6184,6 @@
def _SLASH_JMC : CLIgnoredFlag<"JMC">;
def _SLASH_kernel_ : CLIgnoredFlag<"kernel-">;
def _SLASH_nologo : CLIgnoredFlag<"nologo">;
-def _SLASH_permissive_ : CLIgnoredFlag<"permissive-">;
def _SLASH_RTC : CLIgnoredJoined<"RTC">;
def _SLASH_sdl : CLIgnoredFlag<"sdl">;
def _SLASH_sdl_ : CLIgnoredFlag<"sdl-">;
Index: clang/test/Driver/cl-permissive.c
===
--- /dev/null
+++ clang/test/Driver/cl-permissive.c
@@ -0,0 +1,17 @@
+// Note: %s must be preceded by --, otherwise it may be interpreted as a
+// command-line option, e.g. on Mac where %s is commonly under /Users.
+
+// RUN: %clang_cl /permissive -### -- %s 2>&1 | FileCheck -check-prefix=PERMISSIVE %s
+// PERMISSIVE: "-fno
[PATCH] D103773: [clang-cl] Add /permissive and /permissive-
zero9178 added a comment. I shall do that. I do have commit access, but thanks for asking. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103773/new/ https://reviews.llvm.org/D103773 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D103773: [clang-cl] Add /permissive and /permissive-
thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Since it's motivated by a concrete file, this makes sense to me. Wait maybe a day or so to see if anyone objects, but I think this (and dependencies) are good. Do you have commit access? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103773/new/ https://reviews.llvm.org/D103773 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D103773: [clang-cl] Add /permissive and /permissive-
zero9178 added a comment.
> I think adding /permissive- to make things more conforming is great. The docs
> say "Starting in Visual Studio 2019 version 16.8, the /std:c++latest option
> implicitly sets the /permissive- option." so maybe we should do that too
> (doesn't have to be in this patch).
That's a very good idea, didn't think of that. Should definitely be done.
> Since things seem to mostly work without /permissive (without the -) I'm not
> sure if we should add support for that. It'll make clang-cl less conforming,
> and in practice things seem to be fine as-is (…right?). Part of the reason
> why /permissive- doesn't expand to more flags in clang-cl
> (https://docs.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance?view=msvc-160
> lists a whole bunch more) I imagine is because clang-cl already has the
> stricter defaults there – which it can do because it's a newer compiler that
> needs to support less old code.
clang-cl definitely has stricter defaults, and I think it should absolutely
stay that way. Whether more of the /Zc: conformance options should be added, I
don't know. But I think /permissive should be there to group them, at least for
`/Zc:twoPhase`- and `-fno-operator-names`.
Regarding the later, there is actually no equivalent flag for the GNU style
`-fno-operator-names` in clang-cl currently. In MSVC it's done via /permissive
and /permissive-. The C++ operator keywords are one of the reason I wrote this
patch. There is a Windows sdk header, Query.h, which due to a workaround inside
of clang-cl can be included, but not fully used. It includes a struct that
looks like the following (from Microsoft Docs):
c
struct tagRESTRICTION
{
ULONG rt;
ULONG weight;
/* [switch_is][switch_type] */ union _URes
{
/* [case()] */ NODERESTRICTION ar;
/* [case()] */ NODERESTRICTION or; // error C2059: syntax error:
'||'
/* [case()] */ NODERESTRICTION pxr;
/* [case()] */ VECTORRESTRICTION vr;
/* [case()] */ NOTRESTRICTION nr;
/* [case()] */ CONTENTRESTRICTION cr;
/* [case()] */ NATLANGUAGERESTRICTION nlr;
/* [case()] */ PROPERTYRESTRICTION pr;
/* [default] */ /* Empty union arm */
} res;
};
It includes a field called `or`. A short program that simply has `#include
` will still compile with clang-cl, but that is due to a sort of
"hack" here:
https://github.com/llvm/llvm-project/blob/4f8bc7caf4e5fcc1620b3fd4980ec8d671e9345b/clang/lib/Lex/Preprocessor.cpp#L720.
Basically any C++ operator keywords inside a system header is treated as
identifier instead. This has caused quite a lot of issues as can be seen here:
https://bugs.llvm.org/show_bug.cgi?id=42427 and here
https://github.com/nlohmann/json/issues/1818. It has also caused issues in
libc++ which contains `and` and `or` in headers, and marks them as system
header using `#pragma GCC system_header`.
Additionally, in the Query.h example, if one where to try to access the `or`
field in the above struct, one will instead get an error, as `or` is treated as
a keyword in the source file. The MSVC documented way of fixing this would be
/permissive.
To be fully transparent however, in the header there is a macro called
`QUERY_H_RESTRICTION_PERMISSIVE` which allows one to rename the field. This is
sadly undocumented anywhere else but the header tho.
My intentions with /permissive were to be more consistent with MSVC as well as
the advice given for the various SDK headers and make it possible to remove the
above hack. I was going to put that into an additional patch however, as that
is probably worth a discussion in itself since it's a possibly breaking change
for users of Query.h.
> Details:
> docs say "You can pass specific /Zc options after /permissive- on the command
> line to override this behavior" – does that work with this approach? We
> should have a test for that
The test I added contains tests overwriting `/Zc:twoPhase` for both
`/permissive` and `/permissive-` at line 11 on wards. I could add more tests if
needed however.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103773/new/
https://reviews.llvm.org/D103773
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D103773: [clang-cl] Add /permissive and /permissive-
thakis added a comment. I think adding `/permissive-` to make things more conforming is great. The docs say "Starting in Visual Studio 2019 version 16.8, the /std:c++latest option implicitly sets the /permissive- option." so maybe we should do that too (doesn't have to be in this patch). Since things seem to mostly work without `/permissive` (without the `-`) I'm not sure if we should add support for that. It'll make clang-cl less conforming, and in practice things seem to be fine as-is (…right?). Part of the reason why `/permissive-` doesn't expand to more flags in clang-cl (https://docs.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance?view=msvc-160 lists a whole bunch more) I imagine is because clang-cl already has the stricter defaults there – which it can do because it's a newer compiler that needs to support less old code. Details: - docs say "You can pass specific /Zc options after /permissive- on the command line to override this behavior" – does that work with this approach? We should have a test for that Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103773/new/ https://reviews.llvm.org/D103773 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D103773: [clang-cl] Add /permissive and /permissive-
zero9178 created this revision.
zero9178 added reviewers: rnk, thakis, hans, mstorsjo.
Herald added a subscriber: dang.
zero9178 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This patch adds the command line options /permissive and /permissive- to
clang-cl. These flags are used in MSVC to enable various /Zc language
conformance options at once. In particular, /permissive is used to enable the
various non standard behaviour of MSVC, while /permissive- is the opposite.
When either of two command lines are specified they are simply expanded to the
various underlying /Zc options. In particular when /permissive is passed it
currently expands to:
- /Zc:twoPhase- (disable two phase lookup)
- -fno-operator-names (disable C++ operator keywords)
/permissive- expands to the opposites of these flags + /Zc:strictStrings
(/Zc:strictStrings- does not currently exist). In the future, if any more MSVC
workarounds are ever added they can easily be added to the expansion. One is
also able to override settings done by permissive. Specifying /permissive-
/Zc:twoPhase- will apply the settings from permissive minus, but disables two
phase lookup.
Motivation for this patch was mainly parity with MSVC as well as compatibility
with Windows SDK headers. The /permissive page from MSVC documents various
workarounds that have to be done for the Windows SDK headers [1], when MSVC is
used with /permissive-. In these, Microsoft often recommends simply compiling
with /permissive for the specified source files. Since some of these also apply
to clang-cl (which acts like /permissive- by default mostly), and some are
currently implemented as "hacks" within clang that I'd like to remove, adding
/permissive and /permissive- to be in full parity with MSVC and Microsofts
documentation made sense to me.
[1]
https://docs.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance?view=msvc-160#windows-header-issues
Depends on https://reviews.llvm.org/D103749 for the -foperator-names CLI option
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D103773
Files:
clang/include/clang/Driver/Options.td
clang/lib/Driver/ToolChains/MSVC.cpp
clang/test/Driver/cl-permissive.c
Index: clang/test/Driver/cl-permissive.c
===
--- /dev/null
+++ clang/test/Driver/cl-permissive.c
@@ -0,0 +1,17 @@
+// Note: %s must be preceded by --, otherwise it may be interpreted as a
+// command-line option, e.g. on Mac where %s is commonly under /Users.
+
+// RUN: %clang_cl /permissive -### -- %s 2>&1 | FileCheck
-check-prefix=PERMISSIVE %s
+// PERMISSIVE: "-fno-operator-names"
+// PERMISSIVE: "-fdelayed-template-parsing"
+// RUN: %clang_cl /permissive- -### -- %s 2>&1 | FileCheck
-check-prefix=PERMISSIVE-MINUS %s
+// PERMISSIVE-MINUS-NOT: "-fno-operator-names"
+// PERMISSIVE-MINUS-NOT: "-fdelayed-template-parsing"
+
+// The switches set by permissive may then still be manually enabled or
disabled
+// RUN: %clang_cl /permissive /Zc:twoPhase -### -- %s 2>&1 | FileCheck
-check-prefix=PERMISSIVE-OVERWRITE %s
+// PERMISSIVE-OVERWRITE: "-fno-operator-names"
+// PERMISSIVE-OVERWRITE-NOT: "-fdelayed-template-parsing"
+// RUN: %clang_cl /permissive- /Zc:twoPhase- -### -- %s 2>&1 | FileCheck
-check-prefix=PERMISSIVE-MINUS-OVERWRITE %s
+// PERMISSIVE-MINUS-OVERWRITE-NOT: "-fno-operator-names"
+// PERMISSIVE-MINUS-OVERWRITE: "-fdelayed-template-parsing"
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -1489,6 +1489,20 @@
DAL.AddJoinedArg(A, Opts.getOption(options::OPT_D), NewVal);
}
+static void TranslatePermissive(Arg *A, llvm::opt::DerivedArgList &DAL,
+const OptTable &Opts) {
+ DAL.AddFlagArg(A, Opts.getOption(options::OPT__SLASH_Zc_twoPhase_));
+ DAL.AddFlagArg(A, Opts.getOption(options::OPT_fno_operator_names));
+ // There is currently no /Zc:strictStrings- in clang-cl
+}
+
+static void TranslatePermissiveMinus(Arg *A, llvm::opt::DerivedArgList &DAL,
+ const OptTable &Opts) {
+ DAL.AddFlagArg(A, Opts.getOption(options::OPT__SLASH_Zc_twoPhase));
+ DAL.AddFlagArg(A, Opts.getOption(options::OPT_foperator_names));
+ DAL.AddFlagArg(A, Opts.getOption(options::OPT__SLASH_Zc_strictStrings));
+}
+
llvm::opt::DerivedArgList *
MSVCToolChain::TranslateArgs(const llvm::opt::DerivedArgList &Args,
StringRef BoundArch,
@@ -1531,6 +1545,12 @@
} else if (A->getOption().matches(options::OPT_D)) {
// Translate -Dfoo#bar into -Dfoo=bar.
TranslateDArg(A, *DAL, Opts);
+} else if (A->getOption().matches(options::OPT__SLASH_permissive)) {
+ // Expand /permissive
+ TranslatePermissive(A, *DAL, Opts);
+} else if (A->getOption().matche
