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

Sun Aug 7 12:16:24 PDT 2022

royjacobson added a comment.

In D130689#3705145 <https://reviews.llvm.org/D130689#3705145>, @thieta wrote:

> In D130689#3705131 <https://reviews.llvm.org/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 fail in a way that makes the patches making use of C++17 features seem at fault.
>>
>> See:
>> https://github.com/llvm/llvm-project/commit/ede96de751224487aea122af8bfb4e82bc54840b#commitcomment-80507826
>> https://reviews.llvm.org/rG32fd0b7fd5ab
>>
>> How would you feel about adding something like
>>
>>   #if defined(__cplusplus) && __cplusplus < 201703L
>>   #error "LLVM requires at least C++17"
>>   #endif
>>
>> to some central header, to make this switch more visible?
>
> I am not opposed to that directly. But this seems a bit dangerous where bots retain the cmakecache - there must be other cases where we can't really protect in this way.
>
> Another approach would be to unset CMAKE_CXX_STANDARD if it's below 17 in cmake directly.
>
> But in general - I am not a huge fan of CI / bots trying to keep the cache around - many weird issues can arise from this.

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 don't know CMake well enough to do it - if you could post a follow up patch I think it would be quite helpful.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689