3 Protobuf typeadapter tests #32
Conversation
|
This PR is for adding unit tests for the implementation in #31 It has 4 commits stacked on top. d8de88d: Adds an integration test package to cover general proto type support. 3376ec5: Extends the integration package to test the type adapation works in a round trip. f434e47: Adds wstring tests to the integration tests. be3af18: This fixes a function collision which was identified in the above tests when run against rolling. |
Signed-off-by: Gonzo de Pedro <[email protected]>
* Add a README.md for the typeadapter_protobuf
Signed-off-by: Gonzalo de Pedro <[email protected]>
Signed-off-by: Gonzo de Pedro <[email protected]>
Signed-off-by: Gonzalo de Pedro <[email protected]>
tfoote
force-pushed
the
3-protobuf-typeadapter-test
branch
from
be3af18 to
ad74157
Compare
|
I rebased this one such that it hopefully won't have the same merge issues as #31 too |
|
|
||
| # Default to C++17 | ||
| if(NOT CMAKE_CXX_STANDARD) | ||
| set(CMAKE_CXX_STANDARD 17) |
There was a problem hiding this comment.
The proper way to do this is
target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_17)
There was a problem hiding this comment.
That is the best way to specifically set that. In many of our packages however we've taken this shortcut because adding that specific line to every target starts to become verbose and decreases the maintainability of the CMakeLists file itself. And with us doing loops with logic and generators that debuggability is more important. And for the test binaries the useful exports of the built in are also less valuable.
| @@ -0,0 +1,172 @@ | |||
| # ================================= Apache 2.0 ================================= | |||
| # | |||
| # Copyright (C) 2021 Continental | |||
There was a problem hiding this comment.
Please confirm copyright holder
| @@ -0,0 +1,76 @@ | |||
| /* ================================ Apache 2.0 ================================= | |||
| * | |||
| * Copyright (C) 2021 Continental | |||
There was a problem hiding this comment.
PLease confirm copyright holder
| @{ | ||
| # ================================= Apache 2.0 ================================= | ||
| # | ||
| # Copyright (C) 2021 Continental |
There was a problem hiding this comment.
Please confirm copyright holder
| @@ -0,0 +1,69 @@ | |||
| /* ================================ Apache 2.0 ================================= | |||
| * | |||
| * Copyright (C) 2021 Continental | |||
There was a problem hiding this comment.
Please confirm copyright holder
There was a problem hiding this comment.
For the copyright comments, they are based on whether the content was copied from somewhere else initially and then evolved into it's current form. Thus the copyright of the original file was preserved. And although almost all the content has changed before being submitted it's not a clear line when it changes over. Since the code is being contributed back under an open source license with agreed upon terms, and the changes are not useful without the rest of the body of work, it's not worth trying to track when the amount of work has changed over so it's just left as a contribution to the original copyright. Because the only real value that ownership provides is the ability to re-licensee it.
|
|
||
| # Default to C++17 | ||
| if(NOT CMAKE_CXX_STANDARD) | ||
| set(CMAKE_CXX_STANDARD 17) |
There was a problem hiding this comment.
That is the best way to specifically set that. In many of our packages however we've taken this shortcut because adding that specific line to every target starts to become verbose and decreases the maintainability of the CMakeLists file itself. And with us doing loops with logic and generators that debuggability is more important. And for the test binaries the useful exports of the built in are also less valuable.
Third PR
Depends on #31 being merged first.
Adds CI, unit tests and integration test to TypeAdapter generation code.