Generic skeleton implementation - Only with Event and Black Box Test with IPC_bridge App#63
Conversation
…l partial restart handling implementation.
Signed-off-by: Abhishek GOYAL <abhishek.goyal@valeo.com>
…aining feedback review points and introduce GTest-based unit tests for generic_skeleton_event
…ementation_Feedback_Review
…_implementation_Feedback_Review Generic skeleton implementation feedback review
…_implementation_Feedback_Review Use standard include guards for portability
…_implementation_Feedback_Review Address review feedback
…_implementation_Feedback_Review Make GenericSkeleton and supporting types publicly accessible via types.h
…_implementation_Feedback_Review Generic skeleton implementation feedback review [Resolve merge conflicts with main]
…_implementation_Feedback_Review Fix ASan ODR violation
…_implementation_Feedback_Review Generic skeleton implementation feedback review
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
There was a problem hiding this comment.
Critical logic and ABI regressions around event notifications and shared memory were found, plus API/FFI cleanup items.
Status: Action Required | Risk: High | Readiness: 3/10 (Needs Significant Work)
Issues Identified & Suggestions
- Fix callback regression and shared-memory ABI issues:
skeleton_event_common.cpp,skeleton.cpp,data_type_meta_info.h - Align FFI and improve event access/cleanup:
sample_allocatee_ptr.rs,sample_sender_receiver.cpp,generic_skeleton.cpp,generic_skeleton_event.h
Review Details
📁 59 files reviewed | 💬 9 comments
👍 / 👎 individual comments to help improve reviews for you
| GetBindingRuntime<lola::IRuntime>(BindingType::kLoLa) | ||
| .GetLolaMessaging() | ||
| .RegisterEventNotificationExistenceChangedCallback( | ||
| QualityType::kASIL_QM, event_fqn_, [this](const bool has_handlers) noexcept { | ||
| SetQmNotificationsRegistered(has_handlers); | ||
| }); | ||
| } | ||
| if (parent_.GetInstanceQualityType() == QualityType::kASIL_B) |
There was a problem hiding this comment.
[Logic] Behavioral regression: QM notification callback no longer registered for ASIL-B instances.
In the old SkeletonEvent::PrepareOffer(), the QM notification callback was registered unconditionally (for all quality types). The ASIL-B callback was then additionally registered if the instance was ASIL-B.
In the new PrepareOfferCommon(), the QM callback is only registered when GetInstanceQualityType() == kASIL_QM. This means for ASIL-B instances, the QM notification callback is never registered, so qm_event_update_notifications_registered_ will never become true. Consequently, Send() will skip NotifyEvent(QualityType::kASIL_QM, ...) for all ASIL-B instances, and QM consumers subscribed to an ASIL-B event will never receive notifications.
Fix: Register the QM callback unconditionally, matching the old behavior:
// Always register QM callback (QM consumers can exist for both QM and ASIL-B instances)
GetBindingRuntime<lola::IRuntime>(BindingType::kLoLa)
.GetLolaMessaging()
.RegisterEventNotificationExistenceChangedCallback(
QualityType::kASIL_QM, event_fqn_, [this](const bool has_handlers) noexcept {
SetQmNotificationsRegistered(has_handlers);
});
if (parent_.GetInstanceQualityType() == QualityType::kASIL_B)
{
// Additionally register ASIL-B callback
...
}Context for Agents
**Behavioral regression: QM notification callback no longer registered for ASIL-B instances.**
In the old `SkeletonEvent::PrepareOffer()`, the QM notification callback was registered **unconditionally** (for all quality types). The ASIL-B callback was then *additionally* registered if the instance was ASIL-B.
In the new `PrepareOfferCommon()`, the QM callback is only registered when `GetInstanceQualityType() == kASIL_QM`. This means for ASIL-B instances, the QM notification callback is never registered, so `qm_event_update_notifications_registered_` will never become `true`. Consequently, `Send()` will skip `NotifyEvent(QualityType::kASIL_QM, ...)` for all ASIL-B instances, and QM consumers subscribed to an ASIL-B event will never receive notifications.
Fix: Register the QM callback unconditionally, matching the old behavior:
```cpp
// Always register QM callback (QM consumers can exist for both QM and ASIL-B instances)
GetBindingRuntime<lola::IRuntime>(BindingType::kLoLa)
.GetLolaMessaging()
.RegisterEventNotificationExistenceChangedCallback(
QualityType::kASIL_QM, event_fqn_, [this](const bool has_handlers) noexcept {
SetQmNotificationsRegistered(has_handlers);
});
if (parent_.GetInstanceQualityType() == QualityType::kASIL_B)
{
// Additionally register ASIL-B callback
...
}
```
File: score/mw/com/impl/bindings/lola/skeleton_event_common.cpp
Line: 70Chain of Thought
- Validation Decision: accepted
- Confidence: 0.97
- Likelihood Score: 5/5
- Likelihood Reason: This is a clear behavioral regression. Any ASIL-B skeleton event instance will never register the QM notification callback, so QM consumers subscribed to ASIL-B events will never receive notifications. The issue is inevitable for all ASIL-B instances.
- Suggested Action: keep
- Tool Calls Used: 8
Reasoning
The comment accurately identifies a behavioral regression introduced by this PR.
Old behavior (BASE - SkeletonEvent::PrepareOffer(), lines 310-325):
The QM notification callback was registered unconditionally — regardless of instance quality type. The ASIL-B callback was additionally registered only for ASIL-B instances.
New behavior (HEAD - SkeletonEventCommon::PrepareOfferCommon(), lines 61-78):
The QM notification callback is registered only when GetInstanceQualityType() == kASIL_QM. The ASIL-B callback is registered only for kASIL_B. This means for ASIL-B instances, the QM callback is never registered.
Impact on Send():
Both the old SkeletonEvent::Send() (BASE line 211) and the new GenericSkeletonEvent::Send() (HEAD line 57) check IsQmNotificationsRegistered() before calling NotifyEvent(QualityType::kASIL_QM, ...). Since the QM callback is never registered for ASIL-B instances, qm_event_update_notifications_registered_ stays false, and QM consumers will never receive notifications from ASIL-B events.
Additional evidence of the bug:
PrepareStopOfferCommon() (lines 84-86) unconditionally unregisters the QM callback (without checking quality type), which matches the old behavior and is asymmetric with the conditional registration in PrepareOfferCommon(). This strongly suggests the conditional registration is a mistake.
The comment is accurate, well-documented, and provides a concrete fix that matches the original behavior.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/skeleton_event_common.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/skeleton_event.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
3. File Snapshot (BASE)
- File:
score/mw/com/impl/bindings/lola/skeleton_event.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
4. File Snapshot (BASE)
- File:
score/mw/com/impl/bindings/lola/skeleton_event.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
5. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/skeleton_event.h - Lines:
275-365
275: }
276:
277: template <typename SampleType>
278: // Suppress "AUTOSAR C++14 A15-5-3" rule findings. This rule states: "The std::terminate() function shall not be called
279: // implicitly". This is a false positive, all results which are accessed with '.value()' that could implicitly call
280: // 'std::terminate()' (in case it doesn't have value) has a check in advance using '.has_value()', so no way for
281: // throwing std::bad_optional_access which leds to std::terminate(). This suppression should be removed after fixing
282: // [Ticket-173043](broken_link_j/Ticket-173043)
283: // coverity[autosar_cpp14_a15_5_3_violation : FALSE]
284: void SkeletonEvent<SampleType>::PrepareStopOffer() noexcept
...
6. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/generic_skeleton_event.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional information
5: * regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
7. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/skeleton_event.h - Lines:
230-292
230: << event_shared_impl_.GetElementFQId();
231: event_shared_impl_.GetParent().DisconnectQmConsumers();
232: }
233:
234: if (slot.IsValidQM() || slot.IsValidAsilB())
235: {
236: return MakeSampleAllocateePtr(
237: SampleAllocateePtr<SampleType>(&event_data_storage_->at(static_cast<std::uint64_t>(slot.GetIndex())),
238: *event_data_control_composite_,
239: slot));
...
| }); | ||
| if (!rollback_result.has_value()) | ||
| { | ||
| ::score::mw::log::LogWarn("lola") |
There was a problem hiding this comment.
[Logic] Type mismatch in the shared-memory reopen path. When was_old_shm_region_reopened_ is true, RegisterGeneric calls OpenEventDataFromOpenedSharedMemory<std::uint8_t>(element_fq_id). However, the generic creation path (CreateEventDataFromOpenedSharedMemory void overload) allocates memory as EventDataStorage<std::max_align_t>. Accessing memory created as EventDataStorage<std::max_align_t> through a pointer obtained via OpenEventDataFromOpenedSharedMemory<std::uint8_t> (which casts to EventDataStorage<std::uint8_t>*) and then later in GenericSkeletonEvent::Allocate() casting to EventDataStorage<std::max_align_t>* via data_storage_.get<StorageType>() is undefined behavior due to strict aliasing violations and potential layout differences.
Fix: Open the shared memory with std::max_align_t to match the creation type:
auto [data_storage, control_composite] = OpenEventDataFromOpenedSharedMemory<std::max_align_t>(element_fq_id);Context for Agents
**Type mismatch in the shared-memory reopen path.** When `was_old_shm_region_reopened_` is true, `RegisterGeneric` calls `OpenEventDataFromOpenedSharedMemory<std::uint8_t>(element_fq_id)`. However, the generic creation path (`CreateEventDataFromOpenedSharedMemory` void overload) allocates memory as `EventDataStorage<std::max_align_t>`. Accessing memory created as `EventDataStorage<std::max_align_t>` through a pointer obtained via `OpenEventDataFromOpenedSharedMemory<std::uint8_t>` (which casts to `EventDataStorage<std::uint8_t>*`) and then later in `GenericSkeletonEvent::Allocate()` casting to `EventDataStorage<std::max_align_t>*` via `data_storage_.get<StorageType>()` is undefined behavior due to strict aliasing violations and potential layout differences.
Fix: Open the shared memory with `std::max_align_t` to match the creation type:
```cpp
auto [data_storage, control_composite] = OpenEventDataFromOpenedSharedMemory<std::max_align_t>(element_fq_id);
```
File: score/mw/com/impl/bindings/lola/skeleton.cpp
Line: 1140Chain of Thought
- Validation Decision: accepted
- Confidence: 0.00
- Likelihood Score: 0/5
- Suggested Action: keep
- Tool Calls Used: 14
Reasoning
Failed to parse validation response
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/skeleton.cpp - Lines:
1120-1160
1120:
1121: return {score::memory::shared::OffsetPtr<void>(data_storage), CreateEventControlComposite(element_fq_id, element_properties)};
1122: }
1123: std::pair<score::memory::shared::OffsetPtr<void>, EventDataControlComposite> Skeleton::RegisterGeneric(
1124: const ElementFqId element_fq_id,
1125: const SkeletonEventProperties& element_properties,
1126: const size_t sample_size,
1127: const size_t sample_alignment) noexcept
1128: {
1129: if (was_old_shm_region_reopened_)
...
2. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/skeleton.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
3. File Snapshot (BASE)
- File:
score/mw/com/impl/bindings/lola/skeleton.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
4. File Lookup
- Filename:
generic_skeleton_event.h
No files found matching 'generic_skeleton_event.h'
5. File Lookup
- Filename:
service_data_storage.h
Found 1 file(s) matching 'service_data_storage.h':
- score/mw/com/impl/bindings/lola/service_data_storage.h
6. File Lookup
- Filename:
event_data_storage.h
Found 1 file(s) matching 'event_data_storage.h':
- score/mw/com/impl/bindings/lola/event_data_storage.h
7. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/generic_skeleton_event.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional information
5: * regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
8. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/generic_skeleton_event.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
9. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/service_data_storage.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
10. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/event_data_storage.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
11. File Lookup
- Filename:
offset_ptr.h
No files found matching 'offset_ptr.h'
12. File Lookup
- Filename:
offset_ptr.h - Max Results:
10
No files found matching 'offset_ptr.h'
| }; | ||
|
|
There was a problem hiding this comment.
[Logic] ABI-breaking change to shared-memory struct. DataTypeMetaInfo::alignment was widened from std::uint8_t (align_of_) to std::size_t (alignment). Since DataTypeMetaInfo is embedded in EventMetaInfo which is stored in shared memory (storage_->events_metainfo_), this changes the memory layout of the shared region. During a rolling deploy, a new skeleton writing the wider field and an old proxy reading the narrow field (or vice versa) will misinterpret the data, causing incorrect alignment calculations and potential memory corruption.
Consider: If rolling upgrades are required, keep the field as std::uint8_t in the shared memory representation and only widen it in local (non-shared) copies, or add a versioning mechanism to the shared memory layout.
Context for Agents
**ABI-breaking change to shared-memory struct.** `DataTypeMetaInfo::alignment` was widened from `std::uint8_t` (`align_of_`) to `std::size_t` (`alignment`). Since `DataTypeMetaInfo` is embedded in `EventMetaInfo` which is stored in shared memory (`storage_->events_metainfo_`), this changes the memory layout of the shared region. During a rolling deploy, a new skeleton writing the wider field and an old proxy reading the narrow field (or vice versa) will misinterpret the data, causing incorrect alignment calculations and potential memory corruption.
Consider: If rolling upgrades are required, keep the field as `std::uint8_t` in the shared memory representation and only widen it in local (non-shared) copies, or add a versioning mechanism to the shared memory layout.
File: score/mw/com/impl/data_type_meta_info.h
Line: 29Chain of Thought
- Validation Decision: accepted
- Confidence: 0.90
- Likelihood Score: 4/5
- Likelihood Reason: The DataTypeMetaInfo struct is embedded in EventMetaInfo, which is stored in shared memory via ServiceDataStorage::events_metainfo_. Widening alignment from uint8_t to size_t changes the struct layout. If old and new versions of the software co-exist (e.g., during a rolling deploy in an automotive/AUTOSAR context), they will have incompatible shared memory layouts. This is a likely issue in systems that require any form of rolling or staged deployment.
- Suggested Action: keep
- Tool Calls Used: 22
Reasoning
The comment's factual claims are fully verified:
- DataTypeMetaInfo::alignment widened: BASE had
std::uint8_t align_of_, HEAD hasstd::size_t alignment. On 64-bit platforms, this changes from 1 byte to 8 bytes. ✓ - Embedded in EventMetaInfo:
event_meta_info.hshowsEventMetaInfocontainsDataTypeMetaInfo data_type_info_as a member. ✓ - Stored in shared memory:
ServiceDataStorage(inservice_data_storage.h) hasscore::memory::shared::Map<ElementFqId, EventMetaInfo> events_metainfo_, which lives in shared memory (ManagedMemoryResource). ✓ - ABI breaking: On a typical 64-bit system, the old
DataTypeMetaInfois {size_t (8B), uint8_t (1B)} = ~16 bytes with padding, while the new one is {size_t (8B), size_t (8B)} = 16 bytes. While the total struct size may happen to be the same due to padding, the field semantics and byte layout differ — old code reading the 1-bytealign_of_field would read different bytes than new code writing the 8-bytealignmentfield. - This change is introduced by this PR: The widening happens directly in this diff.
The comment is actionable: it suggests keeping uint8_t in the shared memory representation or adding a versioning mechanism. This is concrete advice for an automotive middleware framework where ABI stability in shared memory is critical.
Note: On reflection, on many 64-bit platforms, size_of_ (8B) followed by align_of_ (1B) would have 7 bytes of padding, making the struct 16 bytes total. The new struct with two size_t members is also 16 bytes. However, the internal layout differs — the second field starts at the same offset but now occupies 8 bytes vs 1 byte, meaning the high bytes that were previously padding (and could contain garbage) would now be interpreted as part of the alignment value. An old writer storing alignment=4 in a uint8_t field would leave the upper 7 bytes of that slot uninitialized, and a new reader interpreting all 8 bytes as size_t could get a corrupted al...
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/event_meta_info.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (BASE)
- File:
score/mw/com/impl/bindings/lola/event_meta_info.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
3. File Lookup
- Filename:
skeleton_event_common.h
No files found matching 'skeleton_event_common.h'
4. File Lookup
- Filename:
skeleton_event.h
Found 3 file(s) matching 'skeleton_event.h':
- score/mw/com/impl/bindings/lola/skeleton_event.h
- score/mw/com/impl/bindings/mock_binding/skeleton_event.h
- score/mw/com/impl/skeleton_event.h
5. File Lookup
- Filename:
data_type_meta_info.h
Found 1 file(s) matching 'data_type_meta_info.h':
- score/mw/com/impl/bindings/lola/data_type_meta_info.h
6. File Snapshot (BASE)
- File:
score/mw/com/impl/bindings/lola/data_type_meta_info.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
7. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/skeleton_event_common.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
8. File Lookup
- Filename:
skeleton_event_common.cpp
No files found matching 'skeleton_event_common.cpp'
9. File Snapshot (BASE)
- File:
score/mw/com/impl/bindings/lola/skeleton_event.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
10. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/skeleton_event.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
11. File Lookup
- Filename:
storage.h - Max Results:
10
No files found matching 'storage.h'
12. File Lookup
- Filename:
shared_memory_storage.h
No files found matching 'shared_memory_storage.h'
13. File Lookup
- Filename:
service_data_storage.h
Found 1 file(s) matching 'service_data_storage.h':
- score/mw/com/impl/bindings/lola/service_data_storage.h
14. File Snapshot (BASE)
- File:
score/mw/com/impl/bindings/lola/service_data_storage.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
15. File Snapshot (HEAD)
- File:
score/mw/com/impl/bindings/lola/service_data_storage.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
16. File Lookup
- Filename:
version.h - Max Results:
5
No files found matching 'version.h'
|
|
||
| return {score::memory::shared::OffsetPtr<void>(data_storage), CreateEventControlComposite(element_fq_id, element_properties)}; | ||
| } | ||
| std::pair<score::memory::shared::OffsetPtr<void>, EventDataControlComposite> Skeleton::RegisterGeneric( |
There was a problem hiding this comment.
[Logic] Potential data loss from static_cast<std::uint8_t>(sample_alignment). sample_alignment is now size_t (due to the DataTypeMetaInfo field widening), but it's being truncated to std::uint8_t when constructing the DataTypeMetaInfo for insertion into shared memory. For alignment values > 255 (unlikely but now type-permitted), this silently truncates.
| std::pair<score::memory::shared::OffsetPtr<void>, EventDataControlComposite> Skeleton::RegisterGeneric( | |
| const DataTypeMetaInfo sample_meta_info{sample_size, sample_alignment}; |
Since DataTypeMetaInfo::alignment is now size_t, the cast is unnecessary. Remove it.
Context for Agents
**Potential data loss from `static_cast<std::uint8_t>(sample_alignment)`.** `sample_alignment` is now `size_t` (due to the `DataTypeMetaInfo` field widening), but it's being truncated to `std::uint8_t` when constructing the `DataTypeMetaInfo` for insertion into shared memory. For alignment values > 255 (unlikely but now type-permitted), this silently truncates.
```suggestion
const DataTypeMetaInfo sample_meta_info{sample_size, sample_alignment};
```
Since `DataTypeMetaInfo::alignment` is now `size_t`, the cast is unnecessary. Remove it.
File: score/mw/com/impl/bindings/lola/skeleton.cpp
Line: 1123Chain of Thought
- Validation Decision: accepted
- Confidence: 0.97
- Likelihood Score: 3/5
- Likelihood Reason: While alignment values > 255 are extremely rare in practice, the cast is objectively unnecessary and incorrect given the types involved. The guard earlier in the function limits alignment to max_align_t (typically 16), making truncation practically impossible, but the cast is still a latent bug and code smell.
- Suggested Action: keep
- Tool Calls Used: 3
Reasoning
The comment is accurate and actionable:
-
DataTypeMetaInfo::alignment is
std::size_t: Confirmed at HEAD indata_type_meta_info.h(line 27):std::size_t alignment;. This file is new to this PR. -
sample_alignmentparameter issize_t: Confirmed at line 1078 of skeleton.cpp at HEAD:size_t sample_alignment. -
The cast is unnecessary: Line 1111 does
static_cast<std::uint8_t>(sample_alignment)when constructingDataTypeMetaInfo{sample_size, static_cast<std::uint8_t>(sample_alignment)}. Since both the source (sample_alignment) and the target field (DataTypeMetaInfo::alignment) arestd::size_t, the intermediate cast tostd::uint8_tis unnecessary and introduces a narrowing conversion that would silently truncate values > 255. -
This is new code introduced by this PR: The entire
CreateEventDataFromOpenedSharedMemoryfunction is added in this PR (all lines are marked as "added"). TheDataTypeMetaInfostruct is also new. So this is not legacy debt. -
The suggestion is correct: Simply removing the
static_cast<std::uint8_t>()wrapper and passingsample_alignmentdirectly is the right fix since both types match (size_t). -
Practical impact is limited but real: The function has a guard at lines 1082-1090 that asserts
sample_alignment <= alignof(std::max_align_t)(typically 16), so in practice truncation wouldn't occur. However, the cast is still a clear bug - it's the kind of leftover from when alignment may have beenuint8_t, and removing it is trivially correct.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/data_type_meta_info.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (BASE)
- File:
score/mw/com/impl/bindings/lola/skeleton.cpp - Lines:
1020-1040
1020:
1021: // Suppress "AUTOSAR C++14 A15-5-3" rule findings. This rule states: "The std::terminate() function shall not be called
1022: // implicitly". This is a false positive, there is no way for calling std::terminate().
1023: // coverity[autosar_cpp14_a15_5_3_violation : FALSE]
1024: void Skeleton::InitializeSharedMemoryForControl(
1025: const QualityType asil_level,
1026: const std::shared_ptr<score::memory::shared::ManagedMemoryResource>& memory)
1027: {
1028: auto& control = (asil_level == QualityType::kASIL_QM) ? control_qm_ : control_asil_b_;
1029: control = memory->construct<ServiceDataControl>(*memory);
...
| } | ||
|
|
||
| #[repr(C)] | ||
| #[repr(C, align(16))] |
There was a problem hiding this comment.
[Logic] Rust repr alignment change from repr(C) to repr(C, align(16)). Changing the alignment of SampleAllocateePtr<T> in the Rust FFI representation changes its ABI. If C++ code and Rust code share this type across FFI boundaries, both sides must agree on the layout. Verify that the C++ SampleAllocateePtr variant has alignof >= 16 now that mock_binding::SampleAllocateePtr (which uses std::unique_ptr with a score::cpp::callback deleter) replaces std::unique_ptr<SampleType>. If the C++ variant alignment doesn't match 16, this will cause memory corruption across FFI.
Context for Agents
**`Rust repr` alignment change from `repr(C)` to `repr(C, align(16))`.** Changing the alignment of `SampleAllocateePtr<T>` in the Rust FFI representation changes its ABI. If C++ code and Rust code share this type across FFI boundaries, both sides must agree on the layout. Verify that the C++ `SampleAllocateePtr` variant has `alignof >= 16` now that `mock_binding::SampleAllocateePtr` (which uses `std::unique_ptr` with a `score::cpp::callback` deleter) replaces `std::unique_ptr<SampleType>`. If the C++ variant alignment doesn't match 16, this will cause memory corruption across FFI.
File: score/mw/com/impl/plumbing/rust/sample_allocatee_ptr.rs
Line: 71Chain of Thought
- Validation Decision: accepted
- Confidence: 0.60
- Likelihood Score: 3/5
- Likelihood Reason: The concern about ABI mismatch is legitimate for FFI code. However, the existing test suite (
verify_size_and_align!macro with C++ comparisons) already guards against this. The alignment change was likely made deliberately to match the new C++ layout aftermock_binding::SampleAllocateePtrreplacedstd::unique_ptr. A mismatch would be caught by the existing tests. - Suggested Action: modify
- Tool Calls Used: 13
Reasoning
The comment correctly identifies that changing #[repr(C)] to #[repr(C, align(16))] on SampleAllocateePtr<T> is an ABI-changing modification. The Rust file is explicitly designed for FFI layout compatibility with C++ (as stated in the file header).
The comment is technically accurate:
- The change IS introduced by this PR (not legacy debt)
- Both Rust and C++ sides changed - the C++ variant now uses
mock_binding::SampleAllocateePtr<SampleType>instead ofstd::unique_ptr<SampleType>, which may affect alignment - ABI mismatch between Rust and C++ FFI can indeed cause memory corruption
However, there's significant mitigation:
- The Rust test suite includes
verify_size_and_align!macro tests that compare against C++ sizes AND alignment (see test lines 101-127) - There are explicit negative tests for alignment mismatches (lines 176-184)
- These tests would catch any mismatch at test time
The comment is actionable (asking to verify C++ alignment compatibility) and correctly identifies a real concern for FFI code. But the existing test infrastructure partially addresses this - the developer likely changed the alignment because they tested and found the C++ variant needed 16-byte alignment after the type change.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/plumbing/sample_allocatee_ptr.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (HEAD)
- File:
score/mw/com/impl/plumbing/rust/sample_allocatee_ptr.rs
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
3. File Snapshot (BASE)
- File:
score/mw/com/impl/plumbing/sample_allocatee_ptr.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
4. File Snapshot (BASE)
- File:
score/mw/com/impl/plumbing/rust/sample_allocatee_ptr.rs
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
5. File Lookup
- Filename:
sample_allocatee_ptr.h
Found 2 file(s) matching 'sample_allocatee_ptr.h':
- score/mw/com/impl/bindings/lola/sample_allocatee_ptr.h
- score/mw/com/impl/plumbing/sample_allocatee_ptr.h
6. File Lookup
- Filename:
sample_allocatee_ptr.h - Max Results:
10
Found 2 file(s) matching 'sample_allocatee_ptr.h':
- score/mw/com/impl/bindings/lola/sample_allocatee_ptr.h
- score/mw/com/impl/plumbing/sample_allocatee_ptr.h
7. File Lookup
- Filename:
sample_allocatee_ptr.h - Max Results:
10
Found 2 file(s) matching 'sample_allocatee_ptr.h':
- score/mw/com/impl/bindings/lola/sample_allocatee_ptr.h
- score/mw/com/impl/plumbing/sample_allocatee_ptr.h
8. File Lookup
- Filename:
mock_binding - Max Results:
10
No files found matching 'mock_binding'
9. File Lookup
- Filename:
verify_size_and_align - Max Results:
5
No files found matching 'verify_size_and_align'
10. File Lookup
- Filename:
test_utils_rs - Max Results:
5
No files found matching 'test_utils_rs'
| for (const auto& info : in.events) | ||
| { | ||
| // Check for duplicates | ||
| if (skeleton.events_.find(info.name) != skeleton.events_.cend()) |
There was a problem hiding this comment.
[Logic] This check for duplicate event names is incorrect. The skeleton.events_ map is keyed by stable_name, but this lookup uses info.name. If info.name and stable_name differ, this check will not work as intended, potentially missing duplicates or failing to find existing elements.
The emplace call on line 110 correctly checks for duplicate keys (stable_name) using its return value. This check on line 87 is both incorrect and redundant.
I recommend removing lines 87-91. The check on emplace_result.second on line 115 already handles duplicate stable_name insertions correctly. You could enhance the error message there to be more specific about a duplicate element if you wish.
Context for Agents
This check for duplicate event names is incorrect. The `skeleton.events_` map is keyed by `stable_name`, but this lookup uses `info.name`. If `info.name` and `stable_name` differ, this check will not work as intended, potentially missing duplicates or failing to find existing elements.
The `emplace` call on line 110 correctly checks for duplicate keys (`stable_name`) using its return value. This check on line 87 is both incorrect and redundant.
I recommend removing lines 87-91. The check on `emplace_result.second` on line 115 already handles duplicate `stable_name` insertions correctly. You could enhance the error message there to be more specific about a duplicate element if you wish.
File: score/mw/com/impl/generic_skeleton.cpp
Line: 87Chain of Thought
- Validation Decision: accepted
- Confidence: 0.00
- Likelihood Score: 0/5
- Suggested Action: keep
- Tool Calls Used: 10
Reasoning
Failed to parse validation response
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/generic_skeleton_event.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (HEAD)
- File:
score/mw/com/impl/generic_skeleton.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
3. File Lookup
- Filename:
lola_service_type_deployment.h
Found 1 file(s) matching 'lola_service_type_deployment.h':
- score/mw/com/impl/configuration/lola_service_type_deployment.h
4. File Snapshot (HEAD)
- File:
score/mw/com/impl/generic_skeleton_test.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
5. File Lookup
- Filename:
service_element_map.h
Found 1 file(s) matching 'service_element_map.h':
- score/mw/com/impl/service_element_map.h
6. File Snapshot (HEAD)
- File:
score/mw/com/impl/configuration/lola_service_type_deployment.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
7. File Snapshot (HEAD)
- File:
score/mw/com/impl/service_element_map.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
8. File Lookup
- Filename:
binding_service_type_deployment.h
Found 1 file(s) matching 'binding_service_type_deployment.h':
- score/mw/com/impl/configuration/binding_service_type_deployment.h
9. File Snapshot (HEAD)
- File:
score/mw/com/impl/configuration/binding_service_type_deployment.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
10. File Snapshot (HEAD)
- File:
score/mw/com/impl/generic_skeleton.cpp - Lines:
33-52
33: // Helper to fetch the stable event name from the Configuration
34: std::string_view GetEventName(const InstanceIdentifier& identifier, std::string_view search_name)
35: {
36: const auto& service_type_deployment = InstanceIdentifierView{identifier}.GetServiceTypeDeployment();
37:
38: auto visitor = score::cpp::overload(
39: [&](const LolaServiceTypeDeployment& deployment) -> std::string_view {
40: const auto it = deployment.events_.find(std::string{search_name});
41: if (it != deployment.events_.end())
42: {
...
|
|
||
| auto& event = const_cast<GenericSkeletonEvent&>(event_it->second); | ||
|
|
||
| const auto offer_result = skeleton.OfferService(); |
There was a problem hiding this comment.
[Maintainability] const_cast on GenericSkeletonEvent& to call mutating methods. Both the example code and tests use const_cast<GenericSkeletonEvent&>(event_it->second) (or the pointer equivalent) to get a mutable reference from the const map returned by GetEvents(). This is a code smell — if Send() and Allocate() are intended to be called on events, the GetEvents() API should also provide a non-const overload, or a separate GetMutableEvent() accessor should be added. The underlying objects aren't themselves const (they're stored as non-const in the map), so this isn't technically UB, but it's fragile and clutters every call site.
Consider adding a non-const GetEvents() overload (returning EventMap&) or a GetEvent(name) accessor returning a mutable reference. The ServiceElementMap already has a non-const find(), so this would be a small addition that eliminates all 7+ const_cast sites in this PR.
Context for Agents
**`const_cast` on `GenericSkeletonEvent&` to call mutating methods.** Both the example code and tests use `const_cast<GenericSkeletonEvent&>(event_it->second)` (or the pointer equivalent) to get a mutable reference from the const map returned by `GetEvents()`. This is a code smell — if `Send()` and `Allocate()` are intended to be called on events, the `GetEvents()` API should also provide a non-const overload, or a separate `GetMutableEvent()` accessor should be added. The underlying objects aren't themselves const (they're stored as non-const in the map), so this isn't technically UB, but it's fragile and clutters every call site.
Consider adding a non-const `GetEvents()` overload (returning `EventMap&`) or a `GetEvent(name)` accessor returning a mutable reference. The `ServiceElementMap` already has a non-const `find()`, so this would be a small addition that eliminates all 7+ `const_cast` sites in this PR.
File: score/mw/com/example/ipc_bridge/sample_sender_receiver.cpp
Line: 523Chain of Thought
- Validation Decision: accepted
- Confidence: 0.88
- Likelihood Score: 5/5
- Likelihood Reason: The const_cast pattern is pervasive — used in the example code and in 6 test cases, all newly introduced by this PR. Every call site that needs to call Send() or Allocate() on an event obtained from GetEvents() is forced to use const_cast. This is an inevitable maintenance issue that will only grow as more code uses the GenericSkeleton API.
- Suggested Action: modify
- Tool Calls Used: 12
Reasoning
The comment correctly identifies a real API design issue introduced by this PR:
-
Accurate observation:
GenericSkeleton::GetEvents()only has aconstoverload returningconst EventMap&. TheServiceElementMapdoes have both const and non-constfind(), but they're inaccessible through the const reference. So every consumer that needs to call non-const methods (Send(),Allocate()) on aGenericSkeletonEventis forced to useconst_cast. -
Pervasive in this PR: The pattern appears in:
sample_sender_receiver.cppline 521 (example code)generic_skeleton_event_test.cpplines 135, 167, 203, 250, 291, 332 (6 test methods)
All of these are newly added by this PR.
-
The UB claim is slightly overstated: The underlying
events_member inGenericSkeletonis non-const, andstd::mapstoresGenericSkeletonEvent(notconst GenericSkeletonEvent). So theconst_casthere is technically defined behavior — the constness is only projected through the const reference fromGetEvents(). However, this is still a code smell and fragile. -
Actionable fix: Adding a non-const
GetEvents()overload (returningEventMap&) or aGetMutableEvent(name)accessor would eliminate allconst_castusage. TheServiceElementMapalready supports non-const iteration, so this is a simple addition. -
Introduced by this PR: The
RunAsGenericSkeletonmethod and the entiregeneric_skeleton_event_test.cppare new in this PR, as is theGenericSkeletonclass itself. This is the right time to fix the API design.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/generic_skeleton.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (HEAD)
- File:
score/mw/com/impl/generic_skeleton_event.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
3. File Snapshot (HEAD)
- File:
score/mw/com/impl/generic_skeleton.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
4. File Snapshot (HEAD)
- File:
score/mw/com/impl/generic_skeleton_test.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
5. File Lookup
- Filename:
service_element_map.h
Found 1 file(s) matching 'service_element_map.h':
- score/mw/com/impl/service_element_map.h
6. File Snapshot (HEAD)
- File:
score/mw/com/test/shared_memory_storage/shared_memory_storage_application.cpp - Lines:
1-100
1: /*******************************************************************************
2: * Copyright (c) 2026 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
7. File Snapshot (HEAD)
- File:
score/mw/com/impl/service_element_map.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
8. File Snapshot (HEAD)
- File:
score/mw/com/example/ipc_bridge/sample_sender_receiver.cpp - Lines:
510-555
510: {
511: std::cerr << "Unable to construct skeleton: " << create_result.error() << ", bailing!\n";
512: return EXIT_FAILURE;
513: }
514: auto& skeleton = create_result.value();
515:
516: // Retrieve event using its name
517: auto event_it = skeleton.GetEvents().find(event_name);
518: SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(event_it != skeleton.GetEvents().cend(),
519: "Event not found in GenericSkeleton");
...
9. File Lookup
- Filename:
generic_skeleton_event_test.cpp
No files found matching 'generic_skeleton_event_test.cpp'
10. File Snapshot (BASE)
- File:
score/mw/com/example/ipc_bridge/sample_sender_receiver.cpp - Lines:
460-480
460: std::cout << "Stop offering service...";
461: skeleton.StopOfferService();
462: std::cout << "and terminating, bye bye\n";
463:
464: return EXIT_SUCCESS;
465: }
466:
467: template int EventSenderReceiver::RunAsProxy<IpcBridgeProxy, impl::ProxyEvent<MapApiLanesStamped>>(
468: const score::mw::com::InstanceSpecifier&,
469: const score::cpp::optional<std::chrono::milliseconds>,
...
11. File Snapshot (HEAD)
- File:
score/mw/com/impl/generic_skeleton_event_test.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
| auto event_it = skeleton.GetEvents().find(event_name); | ||
| if (event_it != skeleton.GetEvents().cend()) { | ||
| // Get a non-const reference to the event to call non-const methods. | ||
| auto& generic_event = const_cast<score::mw::com::impl::GenericSkeletonEvent&>(event_it->second); |
There was a problem hiding this comment.
[Maintainability] The example code requires const_cast to call non-const methods like Allocate() and Send() on a GenericSkeletonEvent. This is because GenericSkeleton::GetEvents() returns a const EventMap&, and iterating over it provides only const access to the events.
Forcing users to use const_cast is a sign of an API design issue and breaks const-correctness. If users are expected to mutate the events (which they are), the API should provide a non-const way to access them.
Consider adding a non-const overload for GetEvents() or a dedicated non-const accessor method to GenericSkeleton:
// In GenericSkeleton.h
// Option 1: Non-const overload
EventMap& GetEvents() noexcept;
// Option 2: Non-const accessor by name
GenericSkeletonEvent* GetEvent(std::string_view name) noexcept;This would allow for safer and cleaner user code, avoiding const_cast.
Context for Agents
The example code requires `const_cast` to call non-const methods like `Allocate()` and `Send()` on a `GenericSkeletonEvent`. This is because `GenericSkeleton::GetEvents()` returns a `const EventMap&`, and iterating over it provides only `const` access to the events.
Forcing users to use `const_cast` is a sign of an API design issue and breaks const-correctness. If users are expected to mutate the events (which they are), the API should provide a non-const way to access them.
Consider adding a non-const overload for `GetEvents()` or a dedicated non-const accessor method to `GenericSkeleton`:
```cpp
// In GenericSkeleton.h
// Option 1: Non-const overload
EventMap& GetEvents() noexcept;
// Option 2: Non-const accessor by name
GenericSkeletonEvent* GetEvent(std::string_view name) noexcept;
```
This would allow for safer and cleaner user code, avoiding `const_cast`.
File: score/mw/com/design/skeleton_proxy/generic_skeleton/README.md
Line: 70Chain of Thought
- Validation Decision: accepted
- Confidence: 0.97
- Likelihood Score: 5/5
- Likelihood Reason: Every user of GenericSkeleton who needs to send events must use const_cast, as confirmed by both the README example and the actual production code in sample_sender_receiver.cpp (line 521). This is inevitable for any user of this new API.
- Suggested Action: keep
- Tool Calls Used: 6
Reasoning
The comment is accurate and well-supported by the code:
-
GetEvents()only has a const overload: Ingeneric_skeleton.hline 75, the only declaration isconst EventMap& GetEvents() const noexcept;. There is no non-const overload. -
Allocate()andSend()are non-const: Ingeneric_skeleton_event.hlines 35-37, both methods are declared withoutconstqualifier. -
const_castis required: SinceGetEvents()returnsconst EventMap&, even thoughServiceElementMap::find()has a non-const overload (line 72 ofservice_element_map.h), only the const overload is callable on a const reference, which returns aconst_iteratorthat provides only const access to the values. -
Both README and production code use
const_cast: The README example at line 70 and the actual production code insample_sender_receiver.cppat line 521 both useconst_cast<GenericSkeletonEvent&>(event_it->second)to work around this limitation. -
This is all new code introduced by this PR:
generic_skeleton.hdidn't exist in the base commit, so this is a new API design issue, not legacy debt. -
The suggestion is concrete and actionable: The comment proposes two specific solutions - a non-const overload of
GetEvents()or a dedicated non-const accessor methodGetEvent(std::string_view). Either would eliminate the need forconst_cast.
This is a legitimate API design issue in newly introduced code. Forcing users to use const_cast for normal API usage breaks const-correctness and is a clear sign the API needs a non-const access path.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/skeleton_base.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (HEAD)
- File:
score/mw/com/impl/generic_skeleton.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
3. File Snapshot (HEAD)
- File:
score/mw/com/impl/generic_skeleton_event.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
4. File Snapshot (HEAD)
- File:
score/mw/com/example/ipc_bridge/sample_sender_receiver.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
5. File Snapshot (HEAD)
- File:
score/mw/com/impl/service_element_map.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
| #include "score/mw/com/impl/skeleton_event_base.h" | ||
| #include "score/result/result.h" | ||
|
|
||
| #include <string> |
There was a problem hiding this comment.
[Maintainability] The header <string> is included, but the code uses std::string_view. The correct header for std::string_view is <string_view>.
While this might compile due to transitive includes, it's not guaranteed and makes the code less maintainable.
| #include <string> | |
| #include <string_view> |
Context for Agents
The header `<string>` is included, but the code uses `std::string_view`. The correct header for `std::string_view` is `<string_view>`.
While this might compile due to transitive includes, it's not guaranteed and makes the code less maintainable.
```suggestion
#include <string_view>
```
File: score/mw/com/impl/generic_skeleton_event.h
Line: 21Chain of Thought
- Validation Decision: accepted
- Confidence: 0.90
- Likelihood Score: 4/5
- Likelihood Reason: The file includes
<string>but only usesstd::string_view, which requires<string_view>. While<string>may transitively include<string_view>on many implementations, this is not guaranteed by the C++ standard and is a real portability/maintainability concern. - Suggested Action: modify
- Tool Calls Used: 3
Reasoning
The comment's core claim is valid: the file generic_skeleton_event.h includes <string> (line 21) but only uses std::string_view (line 32), not std::string. The correct header for std::string_view is <string_view>.
However, the comment has a minor factual inaccuracy: it states <cstring> is included, when in fact the file includes <string>. The <cstring> header is the C++ wrapper for C's <string.h> (providing memcpy, strlen, etc.), which is different from <string> (providing std::string). Despite this error in the description, the suggestion to replace with <string_view> is correct.
This is a newly added file (the diff shows it going from 0 lines to 44 lines), so this issue is directly introduced by this PR. The suggestion is actionable and specific: replace #include <string> with #include <string_view>.
I'm modifying the comment to fix the factual error about <cstring> vs <string>.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/generic_skeleton_event.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (HEAD)
- File:
score/mw/com/impl/generic_skeleton_event.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
4 participants
Hi @crimson11
As discussed earler Rasing a pull request containing the Event with black-box tests through Example Application.
Rest will done in second pull request(First week of Feb) which includes the complete Event functionality documentation along with tests for both Event and Field
Introduce Generic Skeleton for Events with LoLa Binding and Black-Box Tests
This pull request introduces a generic skeleton implementation (
GenericSkeleton) that allows for runtime-configurable service instances, initially focusing on events. This new functionality enables applications like gateways and bridges to offer services without compile-time knowledge of specific data types, operating instead on raw data. The implementation includes coreGenericSkeletonandGenericSkeletonEventclasses, their abstract binding interfaces, and a concrete LoLa binding implementation.The changes also involve refactoring the
DataTypeMetaInfostruct to a more generic location, updating theSampleAllocateePtrto supportvoidspecializations for type-erased data handling, and introducinglola::SkeletonEventCommonto centralize shared logic between typed and generic LoLa skeleton events. The new feature is integrated into theIPC_bridgeexample application for black-box testing, and comprehensive unit tests are provided for the new components. Field functionality and comprehensive documentation will be addressed in a subsequent pull request.Architecture Diagram:
flowchart TD subgraph Core_COM_API GS["GenericSkeleton (+93 lines)"] GSE["GenericSkeletonEvent (+44 lines)"] end subgraph Core_COM_Impl SB["SkeletonBase"] SEB["SkeletonEventBase"] GSEB["GenericSkeletonEventBinding (+39 lines)"] DTMI[/"DataTypeMetaInfo (+17 lines)"/] GSEBF["GenericSkeletonEventBindingFactory (+64 lines)"] end subgraph LoLa_Binding LS["lola::Skeleton (+126 lines)"] LGSE["lola::GenericSkeletonEvent (+72 lines)"] LSEC["lola::SkeletonEventCommon (+121 lines)"] end GS -- "inherits" --> SB GS -- "manages 0..n" --> GSE GSE -- "inherits" --> SEB GSE -- "uses" --> GSEB GSEBF -- "creates" --> GSEB GS -- "uses" --> GSEBF GSEB <|-- LGSE : "implements" LS -- "implements" --> SB LS -- "registers generic events" --> LGSE : "RegisterGeneric" LGSE -- "aggregates" --> LSEC : "event_shared_impl_" LSEC -- "uses" --> LS : "parent_" LSEC -- "uses" --> DTMI LGSE -- "uses" --> DTMI GSE -- "uses" --> DTMI GSEBF -- "uses" --> DTMIThis summary was automatically generated by @propel-code-bot