[PATCH] D103773: [clang-cl] Add /permissive and /permissive-

[Markus Böck via Phabricator via cfe-commits]

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 <Query.h>` 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