Differential D131465
C++/ObjC++: switch to gnu++17 as the default standard Authored by MaskRay on Aug 8 2022, 10:11 PM.
Details
Clang's default C++ standard is now gnu++17 instead of gnu++14.
Depends on D131464
Diff Detail
Event TimelineComment Actions I'm not opposed to this, but it should have a community RFC to give people notice that we'd like to change the default in case this will cause issues and people need a bit more time. It's also worth pointing out that GCC has already switched to gnu++17 as their default. Comment Actions Posted "C++/ObjC++: switch to gnu++17 as the default dialect" https://discourse.llvm.org/t/c-objc-switch-to-gnu-17-as-the-default-dialect/64360 Comment Actions Precomit CI found valid failures that seem plausibly related to your changes, but Windows and Debian found different failures which is a bit of a surprise to me. Can you check those?
Comment Actions Rebase. This shall fix clang/test/SemaCXX/new-delete.cpp Update clang-tools-extra/clangd/unittests/SelectionTests.cpp @sammccall
Comment Actions Thanks for doing this, clangd change LGTM.
Comment Actions This one looks to still be failing on Windows according to precommit CI. Otherwise, the changes LGTM
Comment Actions I've improved the test and pinned it to C++17 in 8af74da5bdbdccf13de84a4610ef75cd3dbac09e, you can just revert the changes to it in this patch. Comment Actions revert a clangd change after sammccall's change to make it immune to gnu++14/gnu++17 default change.
Comment Actions I've reverted this change because it causes major llvm-test-suite breakage. You likely need to pin many tests to use -std=c++14. Comment Actions https://github.com/llvm/llvm-test-suite/commit/b3a89445dc4aed7ee9825e024c32d417e36a3f13 should fix llvm-test-suite. Comment Actions If it helps here is a failed build from our bots: There are others but just combinations of the same tests. Example error from the hexxagon build: /home/tcwg-buildbot/worker/clang-armv8-lld-2stage/test/test-suite/MultiSource/Applications/hexxagon/hexxagonboard.cpp:254:10: error: reference to 'empty' is ambiguous
return empty;
^
/home/tcwg-buildbot/worker/clang-armv8-lld-2stage/test/test-suite/MultiSource/Applications/hexxagon/hexxagonboard.h:42:2: note: candidate found by name lookup is 'empty'
empty,
^Comment Actions Thanks! It looks like the switch to -std=c++17 has some major compile-time impact: http://llvm-compile-time-tracker.com/compare.php?from=047c7aa96dadf8a2c71a29e2df610d628d9e7e3e&to=3e99b8d947ac024831e59be2b604ac67a24fed94&stat=instructions The O0 build of 7zip becomes 80% slower (without codegen changes). Comment Actions I suspect this is due to increased STL size in in C++17. Taking one sample file, preprocessed source is 289149 bytes with -std=gnu++14 and 654888 with -std=gnu++17. The source size increased by more than a factor of two, so it's not very surprising that the parsing time also increased by the same factor. Comment Actions This cause a large amount failures in our testing with errors such as: error: ISO C++17 does not allow 'register' storage class specifier [-Wregister] error: ISO C++17 does not allow dynamic exception specifications [-Wdynamic-exception-spec] Comment Actions Sorry to hear that. Who is "our" in this case and do you have a suggestion for how you'd like to proceed? Comment Actions You may want to add -std=gnu++14 or -std=c++14 to pin your projects to C++14, instead of relying on the Clang default. Comment Actions Some down stream regression tests.
Probably currently this is the best way. Comment Actions I don't think this is qualified as a request for revert. Isn't it a matter of add -std=gnu++14 to your project?
I disagree. The issue was fixed on llvm-test-suite by pinning projects to 14. They previously used the implicit default. Comment Actions Can confirm: https://godbolt.org/z/EPxn1T6za I did not expect this (and I'm a bit shocked that we had no test failures from it). Comment Actions I think it is expected (but I did not know when I made the change) https://learn.microsoft.com/en-us/cpp/build/reference/zc-cplusplus?view=msvc-170 says the default is /std:c++14. Comment Actions I don't recall us sticking with the MSVC defaults the last time we bumped default language versions, but I've not gone back through email to look yet. Regardless, I don't think it's ergonomic for Clang to have a different default language standard for Windows targets -- that means when I run tests, they're all run in C++14 mode but when my coworker runs them, they're all C++17; we will assuredly run into "why does this test behave differently for you than me?" problems from diverging. Not everyone using clang is using clang-cl, so I think clang should default to the same language version for Windows as Linux. I think it's reasonable for clang-cl specifically to default to what MSVC does though. (CC @hans @majnemer @rnk to see if they recall details or have opinions.) Comment Actions I also don't recall the details here, but digging through the history: The last bump was D40948. As maskray checked, the msvc default doesn't seem to have changed.
Clang already has often had different defaults for different targets, such as PS4. Having a different one for windows-msvc, that matches msvc's default, seems reasonable to me. If we changed our default for windows, the "why does this test behave differently for you than me?" problem would still be there, except between clang and msvc colleagues. Comment Actions I guess I view less common targets like PS4 differently than I do common targets like x64 Windows. The number of people likely to notice that PS4 is different from the other targets seems far smaller than the number of people noticing the same for Windows. But I've got no data to back that up, either.
If we're all using clang for the command, we'd all get the same behavior, right? There's no expectation that clang and cl behave the same way, that's what clang-cl is for. I guess I don't see why we care about how MSVC behaves unless passing -fms-compatibility or using clang-cl as the driver and so I don't see why MSVC should dictate our behavior. (e.g., using clang and libc++ on Windows because the programmer doesn't want to use MSVC at all [except for the C runtime, I suppose] is something I believe we want to support, and with such a use case, defaulting to what MSVC defaults to seems surprising to me without other motivation.) Comment Actions Right, last time I think MSVC moved first to C++14. I think it's OK for Clang to be the first mover to C++17. Ultimately, users have many options if they run into breakage: pass /std:c++14, if you are a vendor (Microsoft), reconfigure with CLANG_DEFAULT_STD_CXX. Maybe our move will prompt Microsoft to make the default C++17 in their next release. Comment Actions
Okay, should we key this off the driver mode then (clang-cl vs clang), or on the -fms-compatibility version? Comment Actions
heh. There's a PS4 bot that notices. But the PS4 situation is also different in that there is only one toolchain. When you have multiple toolchains with different defaults, as is the case with Windows, then you have users getting annoyed or possibly into trouble. FTR, we (Sony) did a huge pile of work some years ago to make a lot of the C++ tests dialect-agnostic (run with different values of -std, do a lot of #if __cplusplus <= ... for different dialects, like that). There are really very few tests that actively care what the default is. I think we just very recently added one specific to PS4. I am not surprised that there isn't an equivalent for Windows. Comment Actions Does clang-cl set the -fms-compatibility version? If so, then it seems like we'd probably want to key it off -fms-compatibility? Comment Actions I think both driver share the logic for MSVC version detection, which checks for cl.exe on PATH and looks at the registry if that fails. I think we should try to lean towards making this policy easy to document and communicate to users, and that means our rules need to be concise. I worry that different defaults will surprise cross-platform projects ("hey, my code built with on clang Linux, but not Windows with clang-cl, why is that, let's debug..."). A simple rule like "clang 16.0 raised the default C++ standard version to 17, except on PS4" seems preferable. Comment Actions If it's based on installed MSVC version, there's even the risk of "clang-cl on windows doesn't behave the same whether MSVC is installed or not" or "clang-cl on windows doesn't behave the same when MSVC is not registered in the registry (e.g. a zip unpack)" or "clang-cl on Linux (cross-compile) doesn't behave the same as clang-cl on windows". None of which are desirable. Comment Actions Well it has to adapt to MSVC's library files, so it can't always do the same regardless of version. Given that MSVC has supported /std:c++17 for a while, it's probably safe to bump the default, but perhaps not for all versions. Comment Actions Agreed that a simple rule is a good target for us to aim for. I tend to agree. Perhaps the simple rule we're going for is "when executed on Windows, Clang defaults to C++17 unless it is executed from a context in which an MSVC library can be detected, in which case Clang defaults to the same language standard as used by that version of MSVC"? It's a bit long-winded, but the idea being that we try to match MSVC's default when compiling against MSVC libraries and otherwise stick with the clang default. Comment Actions But the last sentence is basically what we have today. When targeting windows-msvc, we try to match the behavior of the msvc version being targeted, which is in turn determined by what flags the user passes, what msvc installation is found in the environment, or the default (19.14). Comment Actions Hmmm, you're right. I think I may have confused myself with the behaviors here -- MSVC still reports __cplusplus as 199711L but they seem to accept a random smattering of C++17 by default and mostly accept C++14, at least from some simple testing: https://godbolt.org/z/dxjq5Wocx So it's a bit hard to say what kind of compatibility we're aiming for there... | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Huh, i actually think something is wrong here (ignoreImplicit and outerImplicit seem to disagree on whether the CXXConstructExpr is implicit).
In any case, this is already wrong today in C++17 mode.
Would you mind adding // FIXME: is CXXConstructExpr implicit or not and I'll look at it later?