-
Notifications
You must be signed in to change notification settings - Fork 177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved readme #231
base: main
Are you sure you want to change the base?
Improved readme #231
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The added CMake example has issues on almost every single line.
- [Lexer](#Lexer) | ||
- [Range over input](#Range-over-input) | ||
- [Unicode](#Unicode) | ||
- [Integration](#Integration) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ToC is already provided by GitHub's UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using the github.com website that's why I created the ToC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't know that
set(CMAKE_CXX_STANDARD 20) | ||
set(CMAKE_CXX_STANDARD_REQUIRED True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake 3.8 does not know about C++20, you must not use this value. Please read the docs:
New in version 3.12
This is not the way to set the standard requirement of a target, you must use compile features for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh,
For compilers that have no notion of a standard level, such as Microsoft Visual C++ before 2015 Update 3, this has no effect.
FetchContent_Declare( | ||
ctre | ||
GIT_REPOSITORY https://github.com/hanickadot/compile-time-regular-expressions.git | ||
GIT_TAG 95c63867bf0f6497825ef6cf44a7d0791bd25883 # v3.4.1 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The include is missing and please read the docs:
New in version 3.11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I totally forgot that
include(FetchContent)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FetchContent_Declare( | |
ctre | |
GIT_REPOSITORY https://github.com/hanickadot/compile-time-regular-expressions.git | |
GIT_TAG 95c63867bf0f6497825ef6cf44a7d0791bd25883 # v3.4.1 | |
) |
GIT_TAG 95c63867bf0f6497825ef6cf44a7d0791bd25883 # v3.4.1 | ||
) | ||
|
||
FetchContent_MakeAvailable(ctre) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if the package was not found we could do
(pseudocode)
find_package(ctre)
if(package not found){
FetchContent(ctre)
include_target_directories(ctre dir)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not the responsibility of this project. There are many ways to "polyfill" a find_package
call. It should instead just use idiomatic CMake in examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess
FetchContent_MakeAvailable(ctre) | |
find_package(ctre REQUIRED) | |
target_link_libraries(project_target PRIVATE ctre::ctre) |
) | ||
|
||
FetchContent_MakeAvailable(ctre) | ||
include_directories("${ctre_SOURCE_DIR}/single-header") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use directory scope commands. Prefer target_include_directories.
add_executable(${PROJECT_NAME} main.cpp) | ||
target_link_libraries(${PROJECT_NAME}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add_executable(${PROJECT_NAME} main.cpp) | |
target_link_libraries(${PROJECT_NAME}) | |
add_executable(example main.cpp) | |
target_link_libraries(example PRIVATE ctre::ctre) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that the CMake example would be more of a template if we did
add_executable(${PROJECT_NAME} main.cpp) | |
target_link_libraries(${PROJECT_NAME}) | |
add_executable(${PROJECT_NAME} main.cpp) | |
target_link_libraries(${PROJECT_NAME} PRIVATE ctre::ctre) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ${PROJECT_NAME}
is pointless either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${PROJECT_NAME}
makes it so there are fewer things to rewrite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Find and replace exists. How often does the project name change that this abstraction needs to exist? This also just obfuscates the target's name. There are only downsides to using this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The project name changes every time someone copies the CMake example template and puts it in their own project. I would say that anyone wanting to not use the variable ${PROJECT_NAME}
is already further along in their project where they would only need to Ctrl+C the middle portion of the template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is just for a copy-paste example, then using an explicit name makes no semantic difference, but syntactically aligns better with what one should write to begin with. Anyone sufficiently knowledgeable regarding CMake will not bother with this example, as the only interesting thing is the exported target(s) in the install interface.
FetchContent_MakeAvailable(ctre) | ||
include_directories("${ctre_SOURCE_DIR}/single-header") | ||
|
||
# Add an executable with the above sources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with the above sources
What sources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ctre
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctre is a header only library, it has no source files and to the headers to the include path you use target_link_libraries(project_target PRIVATE ctre::ctre)
(once the appropriate PR is merged).
I added compiler explorer links as well as some better examples.