Expose SamplePtr age in C++ API

[Gfilinic]

I’m interested in contributing to this project and started by looking into Issue #13.

Changes

• updated the constructor to carry the necessary context
• added a GetTimestamp() function as requested
• implemented a unit test to verify the behavior

Questions

• Are there any specific commit message or PR guidelines I should follow?
• Is there a preferred way to assign or request reviewers?

Design question

For the default/null constructors, I currently initialize the internal state (event_data_ctrl_ = nullptr, empty slot_indicator_, etc.) so that GetTimestamp() safely returns a default value.

Does this align with the intended design, or would you prefer a different approach for representing an "invalid" SamplePtr?

I was advised to reach out to @crimson11 for this part of the code, as he likely has deeper insight into the design.

[LittleHuba]

Adding @darkwisebear and @bharatGoswami8 for input from Rust API side.

[castler]

Thanks for your contribution.

I am the opinion that we should not expose this implementation detail - especially not the implementation data types.

We still want to give the opportuntity for a user to sort the samples.
Thus, we propose that the SamplePtr gets an bool IsNewer(SamplePtr<T> other) function instead.

[Gfilinic]

Update: If I understood correctly, the intention is to avoid exposing EventTimeStamp (and related internals) and instead provide a way to compare samples without leaking those details.

With that in mind, I’ve moved GetTimestamp() to a private helper (so it can still be reused internally if needed) and introduced IsNewer() as the public API for comparison.

Unit tests were updated accordingly --> instead of checking timestamps directly, they now verify ordering by sorting SamplePtr instances using std::sort with IsNewer().

Let me know if this matches what you had in mind or if you'd prefer a different comparison approach.

[Gfilinic]

Update

• Updated rust lola binding signature to match the newly proposed sample_ptr signature (noticed after failing workflow)

[Gfilinic]

Update

• Add API documentation for newly introduced functions
• Use std::uint8_t instead of uint8_t
• Return false in SamplePtr::IsNewer() if either instance is invalid
• Add test helper for creating SamplePtr instances
• Refactor tests into focused cases:
  • newer vs. older comparisons
  • ordering via std::sort
  • invalid SamplePtr scenarios

[Gfilinic]

Thanks for the feedback and the discussion so far.

I revisited the implementation and reworked the approach based on the suggestions.

Changes

• Replaced the initially proposed dedicated function (IsNewer) with standard comparison operators:

  • operator<
  • operator>

• Simplified the internal representation:

  • Instead of storing multiple internal members, the implementation now only keeps the timestamp (EventTimeStamp, underlying uint32_t)
  • Timestamp is captured once during construction

• Updated impl::SamplePtr accordingly to expose the same comparison operators at the abstraction level

Notes on testing (impl::SamplePtr)

I did not introduce additional comparison tests for impl::SamplePtr.

The current plumbing tests rely on mock_binding::SamplePtr, which is defined as a std::unique_ptr alias and does not provide timestamp or comparison semantics.

Because of that:

• comparisons in the plumbing layer would always evaluate to false
• meaningful tests would require turning the mock binding into a full class with timestamp support

I intentionally did not extend the mock binding in this PR to keep the scope focused.

Additional notes

I force-pushed the branch with only these changes to clean up the git history, but I do have a backup branch with the previous implementation in case that approach is preferred.

[Gfilinic]

Update

Added test in impl by implementing actual sampleptr instead of using generic mock pointer.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If this PR is still relevant, please leave a comment or push new changes to keep it open.

[Gfilinic]

Will there be more discussion in this PR or should I leave it be?

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If this PR is still relevant, please leave a comment or push new changes to keep it open.

[LittleHuba]

Hi @Gfilinic,
sorry for the extended silence. The colleague that did the reviews so far is on sick leave and this PR slipped through on the take over. I'll take the review up and will provide you feedback ASAP.

[LittleHuba]

Hi @Gfilinic,
your PR looks good now. It can be merged as it currently is.
Though, ideally I would like an immediate follow-up that provides the implementation of this API in the mock_binding. Otherwise we have an API that is not mockable...
Are you interested in working on that?

All remarks resolved.