Skip to content

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

Merged
wing328 merged 9 commits into
masterfrom
revert-18820-revert-18474-cpp-anyOf
Jun 7, 2024
Merged

[cpp-restsdk] add support for oneOf via std::variant#18821
wing328 merged 9 commits into
masterfrom
revert-18820-revert-18474-cpp-anyOf

Conversation

@wing328
Copy link
Copy Markdown
Member

@wing328 wing328 commented Jun 1, 2024

Reverts #18820

based on #18474

set(CMAKE_POSITION_INDEPENDENT_CODE ON)
set(CXX_STANDARD_REQUIRED ON)

set(CMAKE_CXX_STANDARD 17)
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

aminya commented Jun 2, 2024

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

@wing328
Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread CI/circle_parallel.sh Outdated
Co-authored-by: Amin Yahyaabadi <aminyahyaabadi74@gmail.com>
else()
set(CMAKE_CXX_STANDARD 11)
endif()
endif()
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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.

2 participants