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

[cpp-restsdk] add support for oneOf via std::variant #18821

Merged
merged 9 commits into from
Jun 7, 2024

Conversation

wing328
Copy link
Member

@wing328 wing328 commented Jun 1, 2024

Reverts #18820

based on #18474


set(CMAKE_CXX_STANDARD 17)
Copy link
Member Author

Choose a reason for hiding this comment

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

@aminya did you need to add the above line to CMakeLists file in order to build the client?

I tried the above but no luck.

Copy link
Contributor

@aminya aminya Jun 2, 2024

Choose a reason for hiding this comment

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

No, we don't need this. The few lines below automatically use the latest standard. This should be removed.

#18821 (comment)

@aminya
Copy link
Contributor

aminya commented Jun 2, 2024

What's the failure that's blocking this PR?

@wing328
Copy link
Member Author

wing328 commented Jun 2, 2024

the test failure details can be found inhttps://app.circleci.com/pipelines/github/OpenAPITools/openapi-generator/29877/workflows/eb40bf75-0cfe-4e7d-a276-a6a7b0eb1945/jobs/96564
looks like it's still using C++ 9 somehow.

e.g.

/usr/include/c++/9/variant:1714:8: error: ‘::__enable_hash_call’ has not been declared

Copy link
Contributor

@aminya aminya left a comment

Choose a reason for hiding this comment

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

I would use setup-cpp for setting up the C++ environment

CI/circle_parallel.sh Outdated Show resolved Hide resolved
Co-authored-by: Amin Yahyaabadi <[email protected]>
set(CMAKE_CXX_STANDARD 14)
else()
set(CMAKE_CXX_STANDARD 11)
endif()
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add some logging to be able to see what is happening. The test would need re-generation.

Suggested change
endif()
endif()
message(STATUS "Using C++ standard ${CMAKE_CXX_STANDARD}")

Copy link
Member Author

Choose a reason for hiding this comment

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

looks like it's using C++ 11 according to the error message:

In file included from /home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/cpp-restsdk/client/include/CppRestPetstoreClient/model/CreateUserOrPet_request.h:39:
/usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/variant:116:20: error: no template named 'add_const_t'; did you mean '::std::add_const_t'?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the libstdc++ version. But the compiler standard itself could be different.

Copy link
Contributor

@aminya aminya Jun 5, 2024

Choose a reason for hiding this comment

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

Found the issue! #18821 (comment)

class Category;
class Tag;

#include <variant>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the problem. The include is being rendered inside the namespace instead of top-level

Comment on lines 27 to 28
{{#oneOf}}{{#-first}}
#include <variant>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move the include towards the top so that it is before the namespaces. Probably before or after {{#imports}}?

@aminya
Copy link
Contributor

aminya commented Jun 6, 2024

Awesome. The tests now pass!

@wing328 wing328 marked this pull request as ready for review June 7, 2024 04:24
@wing328 wing328 changed the title Revert "Revert "[cpp-restsdk] add support for oneOf via std::variant"" [cpp-restsdk] add support for oneOf via std::variant Jun 7, 2024
@wing328 wing328 added this to the 7.7.0 milestone Jun 7, 2024
@wing328 wing328 merged commit 4be5971 into master Jun 7, 2024
17 checks passed
@wing328 wing328 deleted the revert-18820-revert-18474-cpp-anyOf branch June 7, 2024 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants