Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Improved readme #231

wants to merge 3 commits into from

Conversation

alexios-angel
Copy link

I added compiler explorer links as well as some better examples.

Copy link

@friendlyanon friendlyanon left a 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)

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.

Copy link
Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, GitHub UI provides a ToC already:

image

Copy link
Author

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

README.md Show resolved Hide resolved
Comment on lines +355 to +356
set(CMAKE_CXX_STANDARD 20)
set(CMAKE_CXX_STANDARD_REQUIRED True)

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.

Copy link
Author

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.

Comment on lines +358 to +362
FetchContent_Declare(
ctre
GIT_REPOSITORY https://github.com/hanickadot/compile-time-regular-expressions.git
GIT_TAG 95c63867bf0f6497825ef6cf44a7d0791bd25883 # v3.4.1
)

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

Copy link
Author

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read the docs:

New in version 3.14

If #230 is merged then this becomes entirely wrong, as using ctre would simply be:

find_package(ctre REQUIRED)
target_link_libraries(project_target PRIVATE ctre::ctre)

Copy link
Author

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)
}

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess

Suggested change
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")

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.

Comment on lines +368 to +369
add_executable(${PROJECT_NAME} main.cpp)
target_link_libraries(${PROJECT_NAME})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
add_executable(${PROJECT_NAME} main.cpp)
target_link_libraries(${PROJECT_NAME})
add_executable(example main.cpp)
target_link_libraries(example PRIVATE ctre::ctre)

Copy link
Author

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

Suggested change
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)

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.

Copy link
Author

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.

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.

Copy link
Author

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.

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

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ctre

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants