investigate: C++17 dependencies for C++14 compatibility (#171)#176
Open
investigate: C++17 dependencies for C++14 compatibility (#171)#176
Conversation
Document std::optional, if constexpr/std::is_same_v, std::apply, structured bindings, attributes, and inline static usage in include/ and src/ with C++14 alternatives and follow-up ticket ideas. Made-with: Cursor
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 21 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA new documentation page is added to Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/investigations/cpp17-cpp14-compatibility.md`:
- Around line 1-206: Trailing whitespace in
docs/investigations/cpp17-cpp14-compatibility.md caused the CI pre-commit
failure; remove any trailing spaces in that file, re-stage and commit the
markdown file (ensuring the auto-applied formatting changes are included), and
run the trailing-whitespace check (e.g., search for regex '\s+$' or use your
repo's pre-commit hooks) to confirm no matches before pushing so the pipeline
passes.
- Around line 44-68: Add a short comparative subsection to the `std::optional`
section that evaluates the trade-offs of using boost::optional, absl::optional,
or a custom someip::optional; explicitly cover dependency footprint (adding
Boost/Abseil), ABI/stability implications when changing public APIs (impacting
get_e2e_header, DeserializationResult, extract_header and public template
types), maintenance cost of a homegrown optional, and feature-parity differences
(e.g., .value() semantics, nullopt equivalents, monadic helpers, operator* /
operator-> behavior used by header_opt and read_be_* implementations), and
finish with a clear recommendation (preferred choice and migration notes) to
guide decision-makers.
- Around line 192-201: Reorder the proposed follow-up tickets so that "Build /
CI" (ticket 7) and "Optional abstraction" (ticket 1) are listed first (they are
foundational), then renumber the remaining items accordingly; additionally add
three new tickets titled "Dual-standard build testing/validation",
"Documentation updates (build instructions & contributor guide)", and "Static
analysis rules to prevent C++17 creep" and insert them after the Build/Optional
tickets; update the enumerated list and any cross-references to use the new
numbering and include the new ticket titles so reviewers can locate them by name
(e.g., "Build / CI", "Optional abstraction", "Dual-standard build
testing/validation").
- Around line 184-189: Add a testing strategy for the dual-standard approach:
add CI matrix entries that build and run unit/integration tests for both C++14
and C++17 across target compilers/platforms (ensure at least one job compiles
the core with the C++14 CMake/Zephyr flags), add a CI lint/static-analysis job
(e.g., clang-tidy or a custom check) configured to -std=c++14 to detect
accidental use of C++17 features (inline static, structured binding, if
constexpr, etc.), ensure tests exercise code paths using the dual-optional
approach (std::optional vs. bundled optional) and serializer dispatch and
detail::apply behavior under both standards, and add a short coding-standard/PR
checklist entry requiring proof (CI pass) that changes build in both modes to
prevent feature creep.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca3fa9bb-c99a-4e64-b93c-9d867009a47c
📒 Files selected for processing (1)
docs/investigations/cpp17-cpp14-compatibility.md
Comment on lines
+44
to
+68
| ### 1. `std::optional` | ||
|
|
||
| | File | Line(s) | Role in context | C++14 alternative | Straightforward? | Public API impact | | ||
| |------|---------|-----------------|-------------------|------------------|-------------------| | ||
| | `include/someip/message.h` | 22 (`#include`), 133, 147–149, 169 | Optional E2E header on `Message`; `get_e2e_header()` returns `std::optional<e2e::E2EHeader>` | Use `boost::optional`, `absl::optional`, or a minimal `someip::optional` in headers; keep `has_value()`-style API or match chosen type’s API | **Moderate** (touches serialization and all call sites) | **Yes** — return type and member type are public | | ||
| | `include/serialization/serializer.h` | 21, 80–88, 92, 227–234 | `DeserializationResult` stores value in `std::optional<T>`; `read_be_*` return `std::optional<…>` | Same optional abstraction; `get_value()` / `move_value()` can use `*opt` or `.get()` after `has_value()` checks instead of `.value()` | **Straightforward** with a shared optional type | **Yes** — template class in public header | | ||
| | `include/e2e/e2e_protection.h` | 22, 73 | `extract_header()` returns `std::optional<E2EHeader>` | Same as above | **Moderate** (callers in `src/`) | **Yes** | | ||
| | `include/e2e/e2e_header.h` | 20 | Include only (no use in this file) | Remove include, or keep only if switching to a custom optional used here | **Trivial** | **No** | | ||
| | `src/serialization/serializer.cpp` | 436–447, 458, 478–486, 494, 502, 512 | Implementations of `read_be_*` returning `std::optional` | Return same replacement optional type; construct with `nullopt` equivalent | **Straightforward** | **No** (implementation) | | ||
| | `src/e2e/e2e_protection.cpp` | 86 | Returns optional from parsing | Same optional type | **Straightforward** | **No** | | ||
| | `src/e2e/e2e_profiles/standard_profile.cpp` | 135–140 | Local `header_opt`, `has_value()`, `.value()` | Use replacement optional; prefer explicit check + `*` after guard instead of throwing `.value()` | **Straightforward** | **No** | | ||
| | `src/someip/message.cpp` | 84, 118, 169–170, 269, 467, 532 | Copy ctor, length, serialize paths using `has_value()` and `operator->` | Equivalent methods on replacement optional | **Straightforward** | **No** | | ||
|
|
||
| **Example (replace throwing `.value()` after a guard):** | ||
|
|
||
| ```cpp | ||
| // C++17 (current pattern in core) | ||
| if (!header_opt.has_value()) { return false; } | ||
| const E2EHeader& header = header_opt.value(); | ||
|
|
||
| // C++14-friendly (works with boost::optional, absl::optional, or * after assert) | ||
| if (!header_opt) { return false; } | ||
| const E2EHeader& header = *header_opt; | ||
| ``` | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider discussing dependency trade-offs for optional replacement.
The analysis correctly identifies all std::optional usage and the public API impact. However, the recommendation to use boost::optional, absl::optional, or a custom implementation lacks discussion of:
- Dependency implications (Boost/Abseil are significant dependencies to add)
- ABI stability considerations when changing optional types
- Maintenance burden of a custom implementation
- Feature parity comparison (e.g.,
.value()throwing behavior, monadic operations)
Consider adding a subsection comparing these alternatives to help decision-makers choose the appropriate path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/investigations/cpp17-cpp14-compatibility.md` around lines 44 - 68, Add a
short comparative subsection to the `std::optional` section that evaluates the
trade-offs of using boost::optional, absl::optional, or a custom
someip::optional; explicitly cover dependency footprint (adding Boost/Abseil),
ABI/stability implications when changing public APIs (impacting get_e2e_header,
DeserializationResult, extract_header and public template types), maintenance
cost of a homegrown optional, and feature-parity differences (e.g., .value()
semantics, nullopt equivalents, monadic helpers, operator* / operator-> behavior
used by header_opt and read_be_* implementations), and finish with a clear
recommendation (preferred choice and migration notes) to guide decision-makers.
Comment on lines
+184
to
+189
| ## Recommended approach | ||
|
|
||
| 1. **Dual-standard (recommended):** Introduce a documented optional type for headers (`std::optional` when C++17+, otherwise bundled or external optional), refactor serializer dispatch without `if constexpr`, add `detail::apply`, fix structured binding and `inline static`, adjust CMake/Zephyr flags for a **C++14 configuration**, add at least one **CI job** compiling core with C++14. | ||
| 2. **C++14-only:** Same code changes, but drop `std::optional` entirely from public headers in favor of one chosen optional type for all builds. | ||
| 3. **Not feasible:** Only if the project refuses any third-party or bundled optional and cannot accept API churn—in that case staying on C++17 is simpler. | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add testing strategy for dual-standard support.
The recommended dual-standard approach is pragmatic. Consider adding guidance on:
- Testing strategy to ensure both C++14 and C++17 builds remain functional
- Preventing accidental C++17 feature creep (e.g., static analysis, CI checks, coding standards)
- CI matrix coverage (compilers, platforms, standards combinations)
This will help ensure the dual-standard support remains viable long-term.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/investigations/cpp17-cpp14-compatibility.md` around lines 184 - 189, Add
a testing strategy for the dual-standard approach: add CI matrix entries that
build and run unit/integration tests for both C++14 and C++17 across target
compilers/platforms (ensure at least one job compiles the core with the C++14
CMake/Zephyr flags), add a CI lint/static-analysis job (e.g., clang-tidy or a
custom check) configured to -std=c++14 to detect accidental use of C++17
features (inline static, structured binding, if constexpr, etc.), ensure tests
exercise code paths using the dual-optional approach (std::optional vs. bundled
optional) and serializer dispatch and detail::apply behavior under both
standards, and add a short coding-standard/PR checklist entry requiring proof
(CI pass) that changes build in both modes to prevent feature creep.
Comment on lines
+192
to
+201
| ## Proposed follow-up tickets | ||
|
|
||
| 1. **Optional abstraction:** Add `someip/optional.h` (or policy for Boost/absl) and migrate `Message`, `DeserializationResult`, E2E APIs, and `Deserializer::read_be_*`. | ||
| 2. **Serializer:** Replace `if constexpr` / `std::is_same_v` in `serializer.h` with C++14-friendly dispatch (tag dispatch or overload set). | ||
| 3. **RTOS threads:** Add `someip::detail::apply` and replace `std::apply` in FreeRTOS, ThreadX, and Zephyr `thread_impl.h`. | ||
| 4. **ThreadX:** Move `inline static s_registry` definition to a single translation unit for C++14. | ||
| 5. **TCP transport:** Replace structured binding in `tcp_transport.cpp` with explicit `pair` access. | ||
| 6. **Attributes / cleanup:** Replace `[[nodiscard]]` / `[[maybe_unused]]` with C++14-compatible patterns or compiler-specific attributes; remove unused `<optional>` from `e2e_header.h` if still unused. | ||
| 7. **Build / CI:** Make `CMAKE_CXX_STANDARD` (and Zephyr `-std`) configurable; add a C++14 build to the pipeline. | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider ticket ordering and additional follow-up items.
The proposed tickets comprehensively cover the technical work. Consider:
- Reorder by dependency: Tickets 7 (build/CI) and 1 (optional abstraction) should precede others, as they're foundational.
- Add tickets for:
- Testing strategy and validation of dual-standard builds
- Documentation updates (build instructions, contributor guide)
- Static analysis rules to prevent C++17 creep if dual-standard is chosen
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/investigations/cpp17-cpp14-compatibility.md` around lines 192 - 201,
Reorder the proposed follow-up tickets so that "Build / CI" (ticket 7) and
"Optional abstraction" (ticket 1) are listed first (they are foundational), then
renumber the remaining items accordingly; additionally add three new tickets
titled "Dual-standard build testing/validation", "Documentation updates (build
instructions & contributor guide)", and "Static analysis rules to prevent C++17
creep" and insert them after the Build/Optional tickets; update the enumerated
list and any cross-references to use the new numbering and include the new
ticket titles so reviewers can locate them by name (e.g., "Build / CI",
"Optional abstraction", "Dual-standard build testing/validation").
Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an audit of C++17 usage in opensomeip core (
include/andsrc/only) for issue #171.Report:
docs/investigations/cpp17-cpp14-compatibility.mdKey findings
std::optionalis the main public-API concern (Message::get_e2e_header(),E2EProtection::extract_header(),DeserializationResult,Deserializer::read_be_*).if constexpr/std::is_same_vinserialization/serializer.harray helpers → replaceable with tag dispatch or overloads (C++14).std::applyin FreeRTOS, ThreadX, and Zephyrthread_impl.h→ smallindex_sequencehelper.tcp_transport.cpp→pair::first/second.inline statics_registryin ThreadXthread_impl.h→ out-of-line definition in one TU.[[nodiscard]]/[[maybe_unused]]→ drop or compiler-specific attributes.<string_view>,<variant>,<any>,<filesystem>,<shared_mutex>, nested namespaces,std::byte, fold expressions, etc.Conclusion: C++14 is feasible with moderate effort; dual-standard via an optional abstraction + CI C++14 job is the recommended path.
Made with Cursor
Summary by CodeRabbit