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

Mon Aug 8 06:13:50 PDT 2022

thieta added a comment.

In D130689#3706336 <https://reviews.llvm.org/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 the build lab and reacts when subsequent changes break everything like this.

Agreed, I think we need to update the protocol for changing the C++ standard in the future to account for more testing beforehand. I might push some changes to the policy document when all this has settled down to see if we can make sure it will be smoother the time we move to C++20. It's unfortunate that some stuff broke considering we where running some bots before it was merged and it didn't show any errors. And local windows builds for me have been clean as well.

>> Can you try to locally rebuild with this patch https://reviews.llvm.org/D131382 ?
>
> That improves things but the build still isn't clean:
> ...
> (FWIW, I don't know if any of the Windows builders in the lab are building with /WX)

I am not sure either - but at least this removed the problem with the std=c++17 not being passed around correctly.

>> I think all the runtime errors is because of that one above - basically we don't set std=c++17 for any of the compiler-rt projects.
>
> I wasn't building compiler-rt, so no idea why this improved things for me. FWIW, he's my CMake config: `-DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DLLVM_TARGETS_TO_BUILD="X86" -DLLVM_ENABLE_IDE=ON -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" -DLLVM_PARALLEL_COMPILE_JOBS=112 -DLLVM_PARALLEL_LINK_JOBS=16`

That is very odd. I would suspect that the problem was the compiler-rt not getting the right flag. But it seems to influence something else as well?

>> I also think we should merge https://reviews.llvm.org/D131367 for now - we can revert that later on if we think it adds to much complexity, since it will delete the bad cache values automatcially.
>
> Seems reasonable to me.

I landed this now - hopefully that fixes some of the issues seen in the bots - but we might need https://reviews.llvm.org/D131382 as well before they go green. I am not sure what the protocol is here, since @MaskRay suggested the change maybe we should wait for him to land it, or should we get it in ASAP to see if that fixes the bots?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689