Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[cpprestsdk] Fix broken non-mockable builds due to conflicting destructors #732

Merged

Conversation

Jauler
Copy link
Contributor

@Jauler Jauler commented Aug 3, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Seems like #595 might have broken cpprestsdk builds when non-gmockable api is selected as it declares explicitly-defaulted api class destructor, but at the same time the destructor itself is unconditionally defined in api source.

@ravinikam @stkrwork @fvarose @etherealjoy @MartinDelille

@Jauler Jauler changed the title [cpprestsdk] Fix broken non-mockable builds [cpprestsdk] Fix broken non-mockable builds due to conflicting destructors Aug 3, 2018
@etherealjoy
Copy link
Contributor

@Jauler
Did you try to check if it is still working when using mockable APIs?

@@ -51,7 +51,7 @@ public:
~{{classname}}() override;
{{/gmockApis}}
{{^gmockApis}}
virtual ~{{classname}}() = default;
virtual ~{{classname}}();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this impact behavior for mockable codegen?

@Jauler
Copy link
Contributor Author

Jauler commented Aug 8, 2018

Hey,
sorry I have not had a chance to test mockable API's builds

Copy link
Contributor

@etherealjoy etherealjoy left a comment

Choose a reason for hiding this comment

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

I generated the non/mockable APIs and they work.

@etherealjoy etherealjoy added this to the 3.2.1 milestone Aug 12, 2018
@etherealjoy etherealjoy merged commit 0cd6d8c into OpenAPITools:master Aug 12, 2018
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Aug 14, 2018
* master: (32 commits)
  Fixed date formatting in typescript node client (OpenAPITools#786)
  better explain usage (OpenAPITools#794)
  Fix float/double default value in C# generator (OpenAPITools#791)
  Enhancements to documentation generators (samples, default values, etc) (OpenAPITools#790)
  Remove duplicate variable declaration (OpenAPITools#792)
  Issue 758 root resource (OpenAPITools#771)
  Do not declare destructor as default when destructor is explicitly declared. (OpenAPITools#732)
  Fix C# client enum issue (OpenAPITools#774)
  [JavaScript] Update vulnerable dependencies (OpenAPITools#784)
  [Ruby] Fix method split (OpenAPITools#780)
  [Java][jaxrs-jersey] add sample with jaxrs-jersey + openapi v3 (OpenAPITools#778)
  update groupId in pom (OpenAPITools#779)
  [cpp-restsdk] Support multi-line descriptions (OpenAPITools#753)
  [Core] Resolve Inline Models (OpenAPITools#736)
  [gradle] Support nullable system property values (OpenAPITools#764)
  Correct URL for openapi-generator.cli.sh in README.md (OpenAPITools#770)
  Fixed the generation of model properties whose data type is a composed (allOf) schema (OpenAPITools#704)
  [JAX-RS][Spec] Add samples to CircleCI (OpenAPITools#759)
  minor update to python generator usage (OpenAPITools#762)
  [C++][Restbed/Pistache] Added fix for byte array (OpenAPITools#752)
  ...
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants