[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:57 + +* GCC >= 7.1 +* Clang >= 5.0 xbolva00 wrote: > Do we have bots which uses last supported compiler versions? GCC 7.1, Clang > 5.0 and MSVC 16.7. > > It is bad to promise something and

[PATCH] D130689: [LLVM] Update C++ standard to 17

xbolva00 added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:57 + +* GCC >= 7.1 +* Clang >= 5.0 Do we have bots which uses last supported compiler versions? GCC 7.1, Clang 5.0 and MSVC 16.7. It is bad to promise something and then breakages here and

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. Hi - I tried to incorporate all the feedback here and added a post to discourse suggesting tweaks to the developer policy - please have a look and review it: https://discourse.llvm.org/t/rfc-updates-to-developer-policy-around-c-standards-bump/64383 Repository: rG

[PATCH] D130689: [LLVM] Update C++ standard to 17

thakis added a comment. Re rollout: I suggested further up to put this behind an opt-in variable. That would've allowed people to test this on their setups. I still think that's a good suggestion, and it would've identified the MSVC issue at least. Repository: rG LLVM Github Monorepo

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. I think a week or two of moratorium would be good for sure. I think we can table this discussion for now and I will write a RFC post in discourse when I have time and we can discuss the details there. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D130689: [LLVM] Update C++ standard to 17

aaron.ballman added a comment. In D130689#3710340 , @MaskRay wrote: > In D130689#3710291 , @aaron.ballman > wrote: > >> In D130689#3710281 , @royjacobson >> wrote: >>

[PATCH] D130689: [LLVM] Update C++ standard to 17

MaskRay added a comment. In D130689#3710291 , @aaron.ballman wrote: > In D130689#3710281 , @royjacobson > wrote: > >> In D130689#3709834 , @thieta wrote: >> >>> In

[PATCH] D130689: [LLVM] Update C++ standard to 17

aaron.ballman added a comment. In D130689#3710281 , @royjacobson wrote: > In D130689#3709834 , @thieta wrote: > >> In D130689#3709742 , >> @aaron.ballman wrote: >> >>>

[PATCH] D130689: [LLVM] Update C++ standard to 17

royjacobson added a comment. In D130689#3709834 , @thieta wrote: > In D130689#3709742 , @aaron.ballman > wrote: > >> One thing I think would be a definite improvement is to have done an RFC on >> Discourse for

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3709742 , @aaron.ballman wrote: > One thing I think would be a definite improvement is to have done an RFC on > Discourse for these changes so that downstreams have a chance to weigh in on > the impact. The patch was

[PATCH] D130689: [LLVM] Update C++ standard to 17

aaron.ballman added a comment. In D130689#3706424 , @aaron.ballman wrote: > In D130689#3706377 , @thieta wrote: > >> In D130689#3706336 , >> @aaron.ballman wrote: >>

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added inline comments. Comment at: llvm/cmake/modules/CheckCompilerVersion.cmake:16 # _MSC_VER == 1927 MSVC++ 14.27 Visual Studio 2019 Version 16.7 -set(MSVC_MIN 19.20) +set(MSVC_MIN 19.27) set(MSVC_SOFT_ERROR 19.27) glandium wrote: > thieta wrote: > >

[PATCH] D130689: [LLVM] Update C++ standard to 17

glandium added inline comments. Comment at: llvm/cmake/modules/CheckCompilerVersion.cmake:16 # _MSC_VER == 1927 MSVC++ 14.27 Visual Studio 2019 Version 16.7 -set(MSVC_MIN 19.20) +set(MSVC_MIN 19.27) set(MSVC_SOFT_ERROR 19.27) thieta wrote: > glandium wrote: >

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added inline comments. Comment at: llvm/cmake/modules/CheckCompilerVersion.cmake:16 # _MSC_VER == 1927 MSVC++ 14.27 Visual Studio 2019 Version 16.7 -set(MSVC_MIN 19.20) +set(MSVC_MIN 19.27) set(MSVC_SOFT_ERROR 19.27) glandium wrote: > You didn't update

[PATCH] D130689: [LLVM] Update C++ standard to 17

glandium added inline comments. Comment at: llvm/cmake/modules/CheckCompilerVersion.cmake:16 # _MSC_VER == 1927 MSVC++ 14.27 Visual Studio 2019 Version 16.7 -set(MSVC_MIN 19.20) +set(MSVC_MIN 19.27) set(MSVC_SOFT_ERROR 19.27) You didn't update

[PATCH] D130689: [LLVM] Update C++ standard to 17

mstorsjo added a comment. In D130689#3700094 , @thakis wrote: > In D130689#3696186 , @thieta wrote: > >> @nikic @thakis I fixed this issue in https://reviews.llvm.org/D131063 and it >> can be built with

[PATCH] D130689: [LLVM] Update C++ standard to 17

mehdi_amini added a comment. > There should be a better way than this. Comprehensive pre-merge testing of > all PRs etc. We already have pre-commit tests on Phabricator on Windows and Linux, but that's not exhaustive and for many reasons I don't believe this is realistic in any way: we will

[PATCH] D130689: [LLVM] Update C++ standard to 17

Trass3r added a comment. In D130689#3707296 , @mehdi_amini wrote: > land the change, wait for a couple of hours to ensure that all bots have > picked up the revision, then revert There should be a better way than this. Comprehensive pre-merge testing

[PATCH] D130689: [LLVM] Update C++ standard to 17

thakis added a comment. Re MSVC: For what it's worth, our Chromium windows bot (which builds trunk llvm and then chromium with that) is happy. It builds with MSVC 2019. Here's the build log (which includes cmake invocations):

[PATCH] D130689: [LLVM] Update C++ standard to 17

mehdi_amini added a comment. > Agreed, I think we need to update the protocol for changing the C++ standard > in the future to account for more testing beforehand. The way I would do it is: wait for a Sunday so that the bots aren't loaded, land the change, wait for a couple of hours to ensure

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3706424 , @aaron.ballman wrote: > +1, thank you for thinking about how we can improve this process in the > future! Given that C++17 adoption across compilers has been far better than > C++20, I suspect the next time

[PATCH] D130689: [LLVM] Update C++ standard to 17

aaron.ballman added a comment. In D130689#3706377 , @thieta wrote: > In D130689#3706336 , @aaron.ballman > wrote: > >> That's the only reason this hasn't been reverted already. Landing sweeping >> changes on a

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3706336 , @aaron.ballman wrote: > That's the only reason this hasn't been reverted already. Landing sweeping > changes on a weekend is a good way to reduce the pain, but we really need to > be sure someone watches

[PATCH] D130689: [LLVM] Update C++ standard to 17

aaron.ballman added a comment. In D130689#3706303 , @thieta wrote: > In D130689#3706263 , @aaron.ballman > wrote: > >> > > > >> Something odd is going on here and we might want to consider a revert of >> this

[PATCH] D130689: [LLVM] Update C++ standard to 17

jhenderson added a comment. FWIW, I've got a Visual Studio configuration (admittedly using a direct Visual Studio generator, not ninja), and I don't have any issues - C++17 is being used. However, I currently only have LLD as an additional project enabled, and don't build compiler-rt.

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3706263 , @aaron.ballman wrote: > > Something odd is going on here and we might want to consider a revert of this > patch until we resolve it. When I do a git pull and cmake files change, > Visual Studio's

[PATCH] D130689: [LLVM] Update C++ standard to 17

aaron.ballman added a comment. In D130689#3706276 , @barannikov88 wrote: > In D130689#3706263 , @aaron.ballman > wrote: > >> The failures I am getting are the same as what's shown by the sanitizer bot >> for

[PATCH] D130689: [LLVM] Update C++ standard to 17

barannikov88 added a comment. In D130689#3706263 , @aaron.ballman wrote: > The failures I am getting are the same as what's shown by the sanitizer bot > for Windows: > https://lab.llvm.org/buildbot/#/builders/127/builds/33980/steps/4/logs/stdio >

[PATCH] D130689: [LLVM] Update C++ standard to 17

aaron.ballman added a comment. In D130689#3706200 , @thieta wrote: > In D130689#3706199 , @cor3ntin > wrote: > >> Trying to read the logs,, notably >>

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3706199 , @cor3ntin wrote: > Trying to read the logs,, notably > `C:\PROGRA~2\MIB055~1\2019\PROFES~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe > `, it would seem that this particular bot is running a version

[PATCH] D130689: [LLVM] Update C++ standard to 17

cor3ntin added a comment. In D130689#3706135 , @aaron.ballman wrote: > In D130689#3705131 , @royjacobson > wrote: > >> This seems to have been more disruptive than expected, since an existing >> CMakeCache.txt

[PATCH] D130689: [LLVM] Update C++ standard to 17

aaron.ballman added a comment. In D130689#3705131 , @royjacobson wrote: > This seems to have been more disruptive than expected, since an existing > CMakeCache.txt can make LLVM compile in previous C++14 configuration. This > seems to make some of the

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3705579 , @Jake-Egan wrote: > There is a failure on the AIX bot also: > ... > https://lab.llvm.org/buildbot/#/builders/214/builds/2707/steps/5/logs/stdio Filed an issue here:

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3705474 , @dyung wrote: > We are seeing an additional failure on an internal linux bot due to the > change to using C++17 by default when using GNU ld: > ... > Switching between BFD ld and gold still fails (although

[PATCH] D130689: [LLVM] Update C++ standard to 17

Jake-Egan added a comment. There is a failure on the AIX bot also: 152.827 [4302/10/270] Linking CXX executable bin/llvm-tblgen FAILED: bin/llvm-tblgen : && /opt/IBM/openxlC/17.1.0/bin/ibm-clang++_r -mcmodel=large -fPIC -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall

[PATCH] D130689: [LLVM] Update C++ standard to 17

dyung added a comment. We are seeing an additional failure on an internal linux bot due to the change to using C++17 by default when using GNU ld: [3/7] Generating GwpAsan-x86_64-Test FAILED: projects/compiler-rt/lib/gwp_asan/tests/GwpAsan-x86_64-Test cd

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3705236 , @royjacobson wrote: > This affects people on their work branches as well, and it's not obvious that > it's a configuration error and not a broken master. > > The CMake approach sounds cleaner to me, but I

[PATCH] D130689: [LLVM] Update C++ standard to 17

royjacobson added a comment. In D130689#3705145 , @thieta wrote: > In D130689#3705131 , @royjacobson > wrote: > >> This seems to have been more disruptive than expected, since an existing >> CMakeCache.txt can

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3705131 , @royjacobson wrote: > This seems to have been more disruptive than expected, since an existing > CMakeCache.txt can make LLVM compile in previous C++14 configuration. This > seems to make some of the bots

[PATCH] D130689: [LLVM] Update C++ standard to 17

royjacobson added a comment. This seems to have been more disruptive than expected, since an existing CMakeCache.txt can make LLVM compile in previous C++14 configuration. This seems to make some of the bots fail in a way that makes the patches making use of C++17 features seem at fault. See:

[PATCH] D130689: [LLVM] Update C++ standard to 17

MaskRay added a comment. In D130689#3704581 , @dyung wrote: > Your change is causing a build failure on the PS4 linux build bot using GCC > 9.3. Can you take a look? > https://lab.llvm.org/buildbot/#/builders/139/builds/26186 > > FAILED: >

[PATCH] D130689: [LLVM] Update C++ standard to 17

Trass3r added a comment. Also fails on gcc 11.2: https://github.com/Trass3r/llvm-project/runs/7703302032 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130689/new/ https://reviews.llvm.org/D130689 ___

[PATCH] D130689: [LLVM] Update C++ standard to 17

dyung added a comment. Your change is causing a build failure on the PS4 linux build bot using GCC 9.3. Can you take a look? https://lab.llvm.org/buildbot/#/builders/139/builds/26186 FAILED:

[PATCH] D130689: [LLVM] Update C++ standard to 17

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb1356504e63a: [LLVM] Update C++ standard to 17 (authored by thieta). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta updated this revision to Diff 450364. thieta added a comment. Fixed spelling error and added links to C++ standard libraries Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130689/new/ https://reviews.llvm.org/D130689 Files:

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. All right! Last call - I am going fix the spelling error and merge this tomorrow EU time unless someone objects. Thanks for all reviews so far! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130689/new/

[PATCH] D130689: [LLVM] Update C++ standard to 17

thakis added a comment. In D130689#3696186 , @thieta wrote: > @nikic @thakis I fixed this issue in https://reviews.llvm.org/D131063 and it > can be built with CXX_STANDARD=17 and OSX_DEPLOYMENT_TARGET=10.11. Thanks! We took this opportunity to bump our

[PATCH] D130689: [LLVM] Update C++ standard to 17

frasercrmck added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:55 +This means that the the following versions are now required to build LLVM +and there is no way to supress this error. + `suppress` Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. @nikic @thakis I fixed this issue in https://reviews.llvm.org/D131063 and it can be built with CXX_STANDARD=17 and OSX_DEPLOYMENT_TARGET=10.11. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130689/new/

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3690274 , @nikic wrote: > Given > https://github.com/llvm/llvm-project/blob/2bb7c54621f31a957302a4deb3d25b752acb07bd/llvm/include/llvm/Support/RWMutex.h#L22-L27, > it seems like this is supposed to be supported. This

[PATCH] D130689: [LLVM] Update C++ standard to 17

nikic added a comment. In D130689#3690258 , @thieta wrote: > In D130689#3689157 , @thakis wrote: > >> Is it expected and intentional that this increases the mac deployment target >> to 10.12? > > I wasn't aware

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3689157 , @thakis wrote: > Is it expected and intentional that this increases the mac deployment target > to 10.12? I wasn't aware of that - but I think it's expected since the check in RWMutex checks for the C++

[PATCH] D130689: [LLVM] Update C++ standard to 17

thakis added a comment. In D130689#3686716 , @thieta wrote: > You can already test this with `-DCMAKE_CXX_STANDARD=17` afaik. I wonder how > many bot owners would actually test this if we made another flag available. Thanks, that works. Our linux and

[PATCH] D130689: [LLVM] Update C++ standard to 17

mehdi_amini added a comment. In D130689#3686760 , @h-vetinari wrote: > My point boils down to: "written using standard C++17 > code" does not sound at all like "core language, no stdlib", but very much > like "core+stdlib". We're allowing C++17

[PATCH] D130689: [LLVM] Update C++ standard to 17

h-vetinari added a comment. My point boils down to: "written using standard C++17 code" does not sound at all like "core language, no stdlib", but very much like "core+stdlib". This is also the first time this split becomes relevant AFAIK, because for moving to C++14, the stdlib was ready

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3686723 , @h-vetinari wrote: > I don't think the standard library can be called a vendor-specific extension, > and so I think this still could/should be made clearer (even though the > status links below would show

[PATCH] D130689: [LLVM] Update C++ standard to 17

h-vetinari added a comment. From the text you quoted: > LLVM subprojects are written using standard C++17 code and avoid unnecessary vendor-specific extensions. I don't think the standard library can be called a vendor-specific extension, and so I think this still could/should be made clearer

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3686718 , @thieta wrote: > In D130689#3684399 , @h-vetinari > wrote: > >> It may be worth calling out that this is about C++17 core language and not >> the standard library?

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3684399 , @h-vetinari wrote: > It may be worth calling out that this is about C++17 core language and not > the standard library? > > libstdcxx only finished C++17 support in GCC 12, and libcxx is still missing >

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3686397 , @thakis wrote: > It'd be nice if this was landed opt-in behind some cmake variable at first, > so that folks could try it out on their bots and see how well things work. You can already test this with

[PATCH] D130689: [LLVM] Update C++ standard to 17

thakis added a comment. It'd be nice if this was landed opt-in behind some cmake variable at first, so that folks could try it out on their bots and see how well things work. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130689/new/

[PATCH] D130689: [LLVM] Update C++ standard to 17

h-vetinari added a comment. It may be worth calling out that this is about C++17 core language and not the standard library? libstdcxx only finished C++17 support in GCC 12, and libcxx is still missing various pieces even today (much less for Clang 5). Repository: rG LLVM Github Monorepo

[PATCH] D130689: [LLVM] Update C++ standard to 17

cor3ntin accepted this revision. cor3ntin added a comment. Thanks a lot for working on this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130689/new/ https://reviews.llvm.org/D130689 ___ cfe-commits

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta updated this revision to Diff 448262. thieta added a comment. Fixed unintended indentation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130689/new/ https://reviews.llvm.org/D130689 Files: bolt/runtime/CMakeLists.txt

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3684334 , @ChuanqiXu wrote: > So it is free that developers want to use some C++17 features in a small > amount of code, right? As soon as this land C++ 17 should be free to use everywhere yeah! Repository: rG

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3684360 , @barannikov88 wrote: > There are a few places (primarily in ADT and Support) that check __cplusplus > > 201402. Do they need to be cleaned up? Sounds good - but maybe that can be addressed in a separate

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta updated this revision to Diff 448261. thieta added a comment. Address some old comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130689/new/ https://reviews.llvm.org/D130689 Files: bolt/runtime/CMakeLists.txt clang/CMakeLists.txt

[PATCH] D130689: [LLVM] Update C++ standard to 17

barannikov88 added a comment. There are a few places (primarily in ADT and Support) that check __cplusplus > 201402. Do they need to be cleaned up? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130689/new/ https://reviews.llvm.org/D130689

[PATCH] D130689: [LLVM] Update C++ standard to 17

mehdi_amini added a comment. In D130689#3684333 , @thieta wrote: > In D130689#3684330 , @mehdi_amini > wrote: > >> What does it mean exactly? We can't use **anything** C++17 without writing >> it in the coding

[PATCH] D130689: [LLVM] Update C++ standard to 17

BertalanD added inline comments. Comment at: llvm/cmake/modules/CheckCompilerVersion.cmake:79-80 set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -std=c++0x") # Test for libstdc++ version of at least 4.8 by checking for _ZNKSt17bad_function_call4whatEv. # Note:

[PATCH] D130689: [LLVM] Update C++ standard to 17

jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. In D130689#3684330 , @mehdi_amini wrote: > Nice, LGTM, thanks for driving this! > >> Remember that if we want to adopt some new feature in a

[PATCH] D130689: [LLVM] Update C++ standard to 17

ChuanqiXu added a comment. In D130689#3684333 , @thieta wrote: > In D130689#3684330 , @mehdi_amini > wrote: > >> What does it mean exactly? We can't use **anything** C++17 without writing >> it in the coding

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta added a comment. In D130689#3684330 , @mehdi_amini wrote: > What does it mean exactly? We can't use **anything** C++17 without writing it > in the coding standards? > I'm not sure it'll be manageable: how do you see this playing out? Probably

[PATCH] D130689: [LLVM] Update C++ standard to 17

mehdi_amini added a comment. Herald added a subscriber: JDevlieghere. Nice, LGTM, thanks for driving this! > Remember that if we want to adopt some new feature in a bigger way it should > be discussed and added to the CodingStandard document. What does it mean exactly? We can't use

[PATCH] D130689: [LLVM] Update C++ standard to 17

thieta created this revision. thieta added reviewers: mehdi_amini, jyknight, jhenderson, hans, tstellar, cor3ntin, MaskRay. Herald added subscribers: ayermolo, StephenFan, mgorny. Herald added a reviewer: rafauler. Herald added a reviewer: Amir. Herald added a reviewer: maksfb. Herald added a