Refactor jsi conversion & NITs#562
Refactor jsi conversion & NITs#562mkopcins merged 6 commits intosoftware-mansion:mainfrom msluszniak:@ms/refactor-jsi
Conversation
| static_assert(std::is_integral<T>::value || std::is_floating_point<T>::value, | ||
| "Only integral and floating-point types are supported"); |
There was a problem hiding this comment.
This assert is deleted because requires meta::IsNumeric<T> guarantees that the function will instantiate only if a value T already meets this condition (meta::isNumeric<T> is implemented as std::is_arithmetic_v<T>), so it is completely redundant.
|
Awaiting for the test plan for the modified files |
…cts/JsiConversions.h
| for (size_t i = 0; i < numShapeDims; ++i) { | ||
| int dim = getValue<int>(shapeArray.getValueAtIndex(runtime, i), runtime); | ||
| tensorView.sizes.push_back(static_cast<int32_t>(dim)); | ||
| int32_t dim = getValue<int32_t>(shapeArray.getValueAtIndex(runtime, i), runtime); |
There was a problem hiding this comment.
Just out of curiosity, why here you opted for int32_t instead of auto?
There was a problem hiding this comment.
because function specialisation for int32_t might return arbitrary type in general. Maybe here a function name kinda suggest that type probably be the same, but there is no guarantee. Here e.g.,
template <typename T>
auto someValue(T value, T diff_value){
return static_cast<float>(value / diff_value);
}result value will be different for different specialization (floating point vs. integer types) but result is always float. But auto is also reasonable option here since we know the implementation of getValue.
On the other hand, when dealing with constructors, modern c++ casts, the resultant type is obvious so you can always safely use auto.
Also if the type of vector tensorView.sizes would be visible here, I'll probably opt to use auto, but here it's not visible at first glance
## Description As in the title, some refactor of jsi. ### Introduces a breaking change? - [ ] Yes - [x] No ### Type of change - [ ] Bug fix (change which fixes an issue) - [ ] New feature (change which adds functionality) - [ ] Documentation update (improves or adds clarity to existing documentation) - [x] Other (chores, tests, code style improvements etc.) ### Tested on - [ ] iOS - [ ] Android ### Testing instructions Needs to be manually testes / unit test (TODO) ### Screenshots <!-- Add screenshots here, if applicable --> ### Related issues <!-- Link related issues here using #issue-number --> ### Checklist - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [ ] My changes generate no new warnings ### Additional notes <!-- Include any additional information, assumptions, or context that reviewers might need to understand this PR. -->
## Description As in the title, some refactor of jsi. ### Introduces a breaking change? - [ ] Yes - [x] No ### Type of change - [ ] Bug fix (change which fixes an issue) - [ ] New feature (change which adds functionality) - [ ] Documentation update (improves or adds clarity to existing documentation) - [x] Other (chores, tests, code style improvements etc.) ### Tested on - [ ] iOS - [ ] Android ### Testing instructions Needs to be manually testes / unit test (TODO) ### Screenshots <!-- Add screenshots here, if applicable --> ### Related issues <!-- Link related issues here using #issue-number --> ### Checklist - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have updated the documentation accordingly - [ ] My changes generate no new warnings ### Additional notes <!-- Include any additional information, assumptions, or context that reviewers might need to understand this PR. -->
3 participants
Description
As in the title, some refactor of jsi.
Introduces a breaking change?
Type of change
Tested on
Testing instructions
Needs to be manually testes / unit test (TODO)
Screenshots
Related issues
Checklist
Additional notes