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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
