Fix some typos #43
Fix some typos #43
Conversation
ClausKlein
requested review from
JeffGarland,
neatudarius,
robert-andrzejuk and
wusatosi
as code owners
| # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
|
|
||
| cmake_minimum_required(VERSION 3.28) | ||
| cmake_minimum_required(VERSION 3.28...4.1) |
There was a problem hiding this comment.
edit: suggestion-removed...
cmake_minimum_required(VERSION 3.28)
I believe on the advice of @nickelpro we are explicitly not putting in ranges
There was a problem hiding this comment.
if on ci is only cmake v3.28, I need this to test import std;
There was a problem hiding this comment.
I'm in favor of version ranges.
There was confusion over what a cmake_minimum_required(VERSION MIN...MAX) meant among some beman maintainers, ie, that it forbid the project from building with a version >MAX, which led to some discussion of avoiding ranges.
This is not the behavior of a range. Instead it offers to CMake a range of behaviors it may emulate. cmake_minimum_required(VERSION 3.28) requires that CMake offer bug-for-bug compatibility with CMake 3.28. cmake_minimum_required(VERSION 3.28...4.1) allows CMake to choose any version in this range to target.
If a maintainer is comfortable with offering a range, we should allow them to do so.
There was a problem hiding this comment.
thanks for the clarification -- that's actually good news :)
CMakePresets.json
Outdated
| "binaryDir": "${sourceDir}/build/${presetName}", | ||
| "cacheVariables": { | ||
| "CMAKE_CXX_STANDARD": "20" | ||
| "CMAKE_CXX_STANDARD": "23" |
There was a problem hiding this comment.
what's the reason for this one? I think we should leave it at 20
There was a problem hiding this comment.
to test import std;
There was a problem hiding this comment.
right, but given the limited compiler support and the hoop jumping required on the cmake side I don't see us putting import std on main yet -- that seems like it needs to wait until we have an official cmake that support import std?
|
other than the questions, looks good |
3 participants
Note: I have tested it with ubuntu-25.04 and llvm-20, works fine!