Add following APIs to facilitate DataSet conversion #659
Conversation
cydrain
commented
Jun 20, 2024
cydrain
commented
- ConvertFromDataTypeIfNeeded()
- ConvertToDataTypeIfNeeded()
* ConvertFromDataTypeIfNeeded() * ConvertToDataTypeIfNeeded() Signed-off-by: Cai Yudong <[email protected]>
|
/kind improvement |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #659 +/- ##
=========================================
+ Coverage 0 71.49% +71.49%
=========================================
Files 0 69 +69
Lines 0 4719 +4719
=========================================
+ Hits 0 3374 +3374
- Misses 0 1345 +1345 |
|
/hold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cydrain, Presburger The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| inline std::shared_ptr<const DataSet> | ||
| ConvertFromDataTypeIfNeeded(const DataSet& ds) { | ||
| if constexpr (std::is_same_v<DataType, typename MockData<DataType>::type>) { | ||
| return ds.shared_from_this(); |
There was a problem hiding this comment.
this is very strange: we don't know who owns the dataset, how can we just create a shared ptr from it? This implicitly requires the passed in ds to be already managed by a shared ptr(not a stack obj, not a raw or unique ptr). This requirement is satisfied for now, but could lead to a hard to find bug in the future.
but this issue is not introduced by this PR, we can figure out a solution and put up a fix later.
There was a problem hiding this comment.
plan to file another PR to change all APIs' input parameter to shared_ptr to avoid this issue completely
|
/lgtm |
|
/unhold |
* ConvertFromDataTypeIfNeeded() * ConvertToDataTypeIfNeeded() Signed-off-by: Cai Yudong <[email protected]>
