Skip to content

Feature/cpp - #152

Merged
chybz merged 27 commits into
cucumber:mainfrom
chybz:feature/cpp
Jun 1, 2023
Merged

Feature/cpp#152
chybz merged 27 commits into
cucumber:mainfrom
chybz:feature/cpp

Conversation

@chybz

@chybz chybz commented May 3, 2023

Copy link
Copy Markdown
Contributor

🤔 What's changed?

  • Added C++ message generation support
  • Added CMake library

⚡️ What's your motivation?

While adding C++ support in Gherkin, saw that other implementations were using this repository, so decided to contribute in here as well.

🏷️ What kind of change is this?

  • ⚡ New feature (non-breaking change which adds new behaviour)

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

@mpkorstanje mpkorstanje left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cheers.

No external dependencies, so no JSON support as of now, hence no tests.

I think we can do without tests for serialization, but I would expect at the very least a test-cpp.yml in the github workflows to ensure the project compiles.

Arbitrary dependency on C++20

I'm not familiar with CPP. Where does the dependency on C++20 come in to play?

Comment thread cpp/src/lib/cucumber-messages/CMakeLists.txt Outdated
@jenisys

jenisys commented May 10, 2023

Copy link
Copy Markdown

Arbitrary dependency on C++20

You should not require the C++20 standard in the CMake file,
if your source code does not require some of the features from the C++20 standard.

POSSIBLE APPROACHES:

  • Define CMAKE_CXX_STANDARD as default if no C++ standard is defined.
    You should support that the user overrides it via cmake -DCMAKE_CXX_STANDARD=17 ... on the command line.
    In addition, this approach allows that a parent CMakeLists.txt file (that wants to use this library as part of the overall build process) predefines the CMAKE_CXX_STANDARD.
  • BETTER APPROACH: Your library(s)/program(s) should specify which C++ features are required.
    This determines which C++ standard is used (or it fails if the C++ compiler does not support these standard(s)/features)
  • The C++ compiler version (as part of the toolchain) defines the default C++ standard that is used by default
# -- FILE: CMakeLists.txt
# EXAMPLE FOR SOLUTION 1
if(NOT DEFINED CMAKE_CXX_STANDARD)
    set(CMAKE_CXX_STANDARD 20)  # Use C++20 standard as default
endif()

@chybz

chybz commented May 10, 2023

Copy link
Copy Markdown
Contributor Author

Yes, definitely. That was just me being lazy. Thank you very much !

@chybz

chybz commented May 31, 2023

Copy link
Copy Markdown
Contributor Author

@mpkorstanje, @jenisys: repository should be in a decent state now, could you please comment on what I should do next ? (A bit more work is necessary for CI/CD). Thank you

@chybz
chybz merged commit 17f4fc4 into cucumber:main Jun 1, 2023
@chybz
chybz deleted the feature/cpp branch June 1, 2023 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants