-
-
Notifications
You must be signed in to change notification settings - Fork 7
Initial C++ library #54
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
base: main
Are you sure you want to change the base?
Conversation
Add rudimentary Nix shell flake
Language and grammar
81c3d26 to
c4ec32e
Compare
|
@seffradev it would be great if you could merge it with main |
| tc_container_terminate(request_id); | ||
| } | ||
|
|
||
| std::expected<void, std::string> run() { |
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.
New to this repo but just thought I'd take a look at what this API may look like as I want to consume the library from C++.
Am I right in thinking the use of std::expected here in this header which a user is expected to include, means that the library is only usable from C++23 and above?
If so I think it may be worth reconsidering, many projects including my one are still on C++20 or C++17.
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.
Good suggestion! If I find the time, I could add the project zeus-cpp/expected with a compilation conditional to either use the polyfill or the standard implementation.
Another thing worth addressing is that I am currently working on an alternative library design using the builder pattern for container creation so future revisions (as this is an early draft) may change.
Thanks for your interest!
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.
Yes, such an polyfill backport could work.
On my own, I would have probably just used Boost:Outcome, but I am too attached to Boost from the past.
oleg-nenashev
left a comment
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.
It would be nice to merge with main after I get #58 over the line.
I do not mind if we merge this experimental codebase first @seffradev . As long as it builds, having a boilerplate in the repo looks reasonable to me, the whole repo is experimental ATM
|
|
||
| add_library(${TARGET} INTERFACE) | ||
|
|
||
| set(CMAKE_CXX_STANDARD 23) |
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.
FTR I am fine with doing only 23 for now, that's not a blocker
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.
My biggest argument against merging what we have is that I have a redesign almost done. I just have some UB to refine away.
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.
Fine by me
What does this PR do?
It implements an initial C++ adapter library design.
It depends on #53.
Why is it important?
It provides idiomatic C++, utilizing RAII patterns.
Related issues