[24603] Add opt-in strict bool normalization#319
Conversation
Signed-off-by: Mirko Morati <mirkomorati@gmail.com>
MiguelCompany
left a comment
There was a problem hiding this comment.
Thank you so much for this well-crafted PR.
Please take the following suggestions into consideration.
8c60452 to
343340b
Compare
Signed-off-by: Mirko Morati <mirkomorati@gmail.com>
343340b to
4c2d0da
Compare
Signed-off-by: Mirko Morati <mirkomorati@gmail.com>
MiguelCompany
left a comment
There was a problem hiding this comment.
Thanks for the great work @mirkomorati
Now, for the final touches:
- Rebase on top of master
- Add a single line
* Add STRICT_BOOL CMake option to enforce canonical bool serializationtoversions.md
It would also be nice if you could add a way of testing the new feature in the CI job. If you don't have experience with GitHub Actions, or you'd prefer we take care of it, we'll do.
|
Thank you for the review! As you guessed I have little to no experience directly with github actions, but I hope the idea is correct:
The Would something like this work? Or were you thinking about exposing a variable diff --git a/.github/workflows/reusable-ci.yml b/.github/workflows/reusable-ci.yml
index e0ab3b5..5ae1c4d 100644
--- a/.github/workflows/reusable-ci.yml
+++ b/.github/workflows/reusable-ci.yml
@@ -68,10 +68,13 @@ jobs:
matrix:
cmake-build-type:
- 'RelWithDebInfo'
+ strict-bool:
+ - false
+ - true
steps:
- name: Add ci-pending label if PR
- if: ${{ github.event_name == 'pull_request' && inputs.add-label == true}}
+ if: ${{ github.event_name == 'pull_request' && inputs.add-label == true && matrix.strict-bool == false }}
uses: eProsima/eProsima-CI/external/add_labels@v0
with:
labels: ci-pending
@@ -122,7 +125,7 @@ jobs:
colcon_meta_file: ${{ github.workspace }}/src/fastcdr/.github/workflows/config/build.meta
colcon_build_args: ${{ inputs.colcon-args }}
colcon_build_args_default: --event-handlers=console_direct+
- cmake_args: ${{ inputs.cmake-args }}
+ cmake_args: ${{ inputs.cmake-args }} ${{ matrix.strict-bool == true && '-DSTRICT_BOOL=ON' || '' }}
cmake_args_default: ${{ env.colcon-build-default-cmake-args }} ${{ env.toolset }}
cmake_build_type: ${{ matrix.cmake-build-type }}
workspace: ${{ github.workspace }}
@@ -137,7 +140,7 @@ jobs:
ctest_args: ${{ inputs.ctest-args }}
packages_names: fastcdr
workspace: ${{ github.workspace }}
- test_report_artifact: ${{ inputs.label }}
+ test_report_artifact: ${{ inputs.label }}${{ matrix.strict-bool == true && '-strict-bool' || '' }}
- name: Fast CDR Test summary
uses: eProsima/eProsima-CI/multiplatform/junit_summary@v0
@@ -153,5 +156,5 @@ jobs:
if: always()
uses: eProsima/eProsima-CI/external/upload-artifact@v0
with:
- name: test-results-${{ inputs.label }}
+ name: test-results-${{ inputs.label }}${{ matrix.strict-bool == true && '-strict-bool' || '' }}
path: log/latest_test/fastcdr |
|
@mirkomorati Looks like the thing I would do |
Description
In release builds, the compiler optimizes the
if (bool_t) { write 1 } else { write 0 }pattern in all theboolserialization paths.This PR introduces a
STRICT_BOOLCMake option (off by default), the relativeFASTCDR_STRICT_BOOLcompiler definition and a newlaunder_bool()helper function that usesmemcpyand a compiler barrier to force normalization of boolean values to 0/1.A dedicated test file
BoolStrictTest.cpphas also been included that tests the "truthy" cases.Since this is an opt-in, we can backport the new feature to 2.3.x, which is the most used branch.
@Mergifyio backport 2.3.x
Fixes #315
Contributor Checklist
versions.mdfile (if applicable).Reviewer Checklist